[PATCH 07/23] Move inode number and generation into ino_gen struct

[Zach Brown]

On Fri, Apr 04, 2025 at 08:45:23PM +0200, Valerie Aurora wrote:
> Files/dirs are uniquely identifed by the combination of inode number
> and generation number. The generation number is incremented every time
> an inode is freed and reallocated. In almost every case, we should use
> both inode and generation number when performing operations. Encourage
> this by using a combination inode number/generation struct. Also
> remove the unused inode number argument from init_dirent_args().

> @@ -268,7 +269,7 @@ static int debugfs_func(int argc, char **argv)
>  	struct ngnfs_fs_info nfi = INIT_NGNFS_FS_INFO;
>  	struct debugfs_context ctx = {
>  		.nfi = &nfi,
> -		.cwd_ino = NGNFS_ROOT_INO,
> +		.cwd_ig = { NGNFS_ROOT_INO, 1 },

Wouldn't this use the INIT_NGNFS_ROOT_IG initializer?

> @@ -52,9 +52,9 @@ static u64 name_hash(void *name, size_t name_len)
>   * copy in and out any dirent as we work with them.
>   */
>  struct dirent_args {
> +	struct ngnfs_inode_ino_gen ig;
>  	u64 hash;
>  	size_t dent_size;
> -	u64 ino;

Do we need this, given that the dent has an ig?  It'd be nice if we
didn't need both.   Maybe that native ino was a bit lazy.. 

> -static int update_dirent_args_ino(struct dirent_args *da, u64 ino)
> +/* caller has filled out da->ig, update the dent.ig to match */
> +static int update_dirent_args_dent(struct dirent_args *da)
>  {
> -	da->dent.ino = cpu_to_le64(ino);
> +	da->dent.ig.ino = cpu_to_le64(da->ig.ino);
> +	da->dent.ig.gen = cpu_to_le64(da->ig.gen);

This went from taking the ino argument, to operating on the ig already
stored in the dirent.  That doesn't seem quite right.. we don't want to
hide all in/out relationships in these arg structs and end up with
functionally global variables that the code paths are operating on.

> +int ngnfs_dir_create(struct ngnfs_fs_info *nfi, struct ngnfs_inode_ino_gen *dir, umode_t mode,
> +		     char *name, size_t name_len)
>  {
>  	struct {
>  		struct ngnfs_transaction txn;
>  		struct ngnfs_inode_txn_ref dir;
>  		struct ngnfs_inode_txn_ref inode;
> -		u64 ino;

If we had an ig here..

> +		      ngnfs_inode_alloc(nfi, &op->txn, &op->da.ig, &op->inode)			?:

Then it could be allocated into here..

> +		      update_dirent_args_dent(&op->da)						?:

And set in the da->dent here, and we wouldn't need the duplicating of
the ig in both da-> and da->dent. .  Maybe?  Hopefully?

> +/*
> + * The inode generation number changes every time a specific inode is
> + * freed and reallocated. The inode number and generation number
> + * uniquely identify a file/dir over its lifetime. This lets users of
> + * references to inodes find out if the inode has changed out from
> + * underneath them. Generally, the inode number and generation number
> + * should be referenced together, so make a struct to keep them
> + * together.
> + */
> +
> +struct ngnfs_ino_gen {
> +	__le64 ino;	/* inode number, starts at 1 */
> +	__le64 gen;	/* inode generation, starts at 1 */
> +};

Loving all the commentary here, thanks for taking the time.

> +int ngnfs_inode_alloc(struct ngnfs_fs_info *nfi, struct ngnfs_transaction *txn,
> +		      struct ngnfs_inode_ino_gen *ig, struct ngnfs_inode_txn_ref *itref)
>  {
> -	u64 bnr;
> +	struct ngnfs_inode_ino_gen new;
>  	int ret;
>  
> -	ret = ngnfs_txn_alloc_meta(txn, &bnr) ?:
> -	      ngnfs_inode_get(nfi, txn, NBF_WRITE | NBF_NEW, bnr, itref);
> +	new.gen = 1; /* XXX should look up previous gen and increment */

Agreed.  FWIW, this'll be in block metadata that's stored by devd, even
for freed blocks, and sent/received over the wire along with block
contents.

- z