[PATCH 17/23] Add ngnfs_dir_rename() and very basic debugfs command

[Zach Brown]

On Fri, Apr 04, 2025 at 08:45:33PM +0200, Valerie Aurora wrote:
> Add mostly functional ngnfs_dir_rename() that removes any existing
> target and checks for directory loops and non-empty target
> directories. The debugfs command is very limited: it can only rename a
> file from the current working directory to the parent directory.

> +static int igs_equal(struct ngnfs_inode_ino_gen *a, struct ngnfs_inode_ino_gen *b)
> +{
> +	return (a->ino == b->ino) && (a->gen == b->gen);

I think we should use _Generic() to compare igs, regardless of which
endian type they're in at the time.

#define _ig_ino(a) \
        _Generic(a, struct ngnfs_ino_gen *: le64_to_cpu(a->ino), \
                    struct ngnfs_node_ino_gen *: a->ino)
#define _ig_gen(a) \
        _Generic(a, struct ngnfs_ino_gen *: le64_to_cpu(a->gen), \
                    struct ngnfs_node_ino_gen *: a->gen)

#define igs_equal(a, b) \
        (_ig_ino(a) == _ig_ino(b) && _ig_gen(a) == _ig_gen(b))

It seemed better to compare the fields than to chose a type to convert
to and have a comparison of structs of that type.  Maybe it results in
better code?  Dunno, all this is totally untested.

> +/*
> + * To prevent loops when renaming a directory, we refuse to rename a
> + * source directory into one of its children. Check by walking up the
> + * path starting from the dst parent directory and comparing each
> + * directory to the src until we get to the root inode. On entry to this
> + * function, src_da is a directory.
> + */
> +static int check_ancestors(struct ngnfs_fs_info *nfi, struct ngnfs_transaction *txn,
> +			   struct ngnfs_inode_txn_ref *src_dir, struct dirent_args *src_da,
> +			   struct ngnfs_inode_txn_ref *dst_dir)
> +{
> +	struct ngnfs_inode_txn_ref walk;
> +	struct ngnfs_inode_ino_gen ig;
> +	int ret;
> +
> +	/* common case: parent dirs the same, therefore no loop */
> +	if ((src_dir->ninode->ig.ino == dst_dir->ninode->ig.ino) &&
> +	    (src_dir->ninode->ig.gen == dst_dir->ninode->ig.gen))
> +		return 0;

igs_equal()

> +static int inodes_equal(struct ngnfs_inode *a, struct ngnfs_inode *b) {
> +       return (a->ig.ino == b->ig.ino) && (a->ig.gen == b->ig.gen);
> +}

And can get rid of this (which would have needed that '{' on a new line).

> +int ngnfs_dir_rename(struct ngnfs_fs_info *nfi, struct ngnfs_inode_ino_gen *src_dir_ig,
> +		     char *src_name, size_t src_name_len, struct ngnfs_inode_ino_gen *dst_dir_ig,
> +		     char *dst_name, size_t dst_name_len)
> +{
> +	struct {
> +		struct ngnfs_transaction txn;
> +		struct ngnfs_inode_txn_ref src_dir, dst_dir, dst;
> +		struct dirent_args src_da, dst_da;
> +	} *op;
> +	int ret;
> +
> +	if ((src_name_len > NGNFS_NAME_MAX) || (dst_name_len > NGNFS_NAME_MAX)) {
> +		ret = -ENAMETOOLONG;
> +		goto out;
> +	}
> +
> +	op = kmalloc(sizeof(*op), GFP_NOFS);
> +	if (!op) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ngnfs_txn_init(&op->txn);
> +	init_dirent_args(&op->src_da, src_name, src_name_len, 0);
> +	init_dirent_args(&op->dst_da, dst_name, dst_name_len, 0);
> +
> +	do {
> +		ret = ngnfs_inode_get(nfi, &op->txn, NBF_WRITE, src_dir_ig, &op->src_dir)	?:
> +		      check_ifmt(op->src_dir.ninode, S_IFDIR, -ENOTDIR)				?:
> +		      lookup_dirent(nfi, &op->txn, &op->src_dir, &op->src_da)			?:
> +		      ngnfs_inode_get(nfi, &op->txn, NBF_WRITE, dst_dir_ig, &op->dst_dir)	?:
> +		      check_ifmt(op->dst_dir.ninode, S_IFDIR, -ENOTDIR)				?:
> +		      lookup_dirent_enoent_ok(nfi, &op->txn, &op->dst_dir, &op->dst_da)		?:
> +		      check_rename(nfi, &op->txn, &op->src_dir, &op->src_da, &op->dst_dir,
> +				   &op->dst_da)							?:
> +		      remove_dirent(nfi, &op->txn, &op->src_dir, &op->src_da)			?:
> +		      update_dir(op->src_dir.tblk, op->src_dir.ninode, &op->src_da, -1)		?:
> +		      replace_dirent(nfi, &op->txn, &op->dst_dir, &op->src_da)			?:
> +		      update_dir(op->dst_dir.tblk, op->dst_dir.ninode, &op->src_da, 1);

We should remove the lookups here and fold their error handling into the
dirents that are found, or not, by the removal of the source and the
insertion/change of the dest.  The btree walks are expensive enough that
it's worth not doing it all twice.

- z