[PATCH 14/14] Add ngnfs_dir_unlink(), ngfs_dir_rmdir(), and debugfs commands

[Zach Brown]

On Thu, Feb 27, 2025 at 03:16:23PM +0100, Valerie Aurora wrote:
> Allow removal of links to files and directories.

> +static int remove_dirent_wr(struct ngnfs_btree_key *key, void *val, size_t size, void *arg,
> +			    struct ngnfs_btree_op *op)
> +{
> +	struct ngnfs_dirent *dent = val;
> +	struct dirent_args *da = arg;
> +	int ret;
> +
> +	if (!dent)
> +		return -ENOENT;
> +
> +	if (!names_equal(dent->name, dent->name_len, da->dent.name, da->dent.name_len))
> +		return NGNFS_BTREE_ITER_CONTINUE;

We need to be careful to return ENOENT if the last key value was
populated and had a colliding hash, but didn't match the name being
removed.  We only get the null val call in write iterators if the very
last key value doesn't exist.  It can exist, and not be the name we're
looking for, and in that case here I think we'll just continue out of
the key range and return success.

> +	/* ino and dtype needed for updating inode and parent directory */
> +	da->ino = le64_to_cpu(dent->ino);
> +	da->dtype = ngnfs_type_to_dtype(dent->type);
> +
> +	ret = check_remove_dirent(dent, da);
> +	if (ret < 0)
> +		return ret;
> +

Hmm, I wonder if there's been a bit of miscommunication.  When we were
talking about checking errors in the btree write iterator, the only
errors I meant were those related to the btree items that are being
iterated over :).  It lets us return ENOENT/EEXIST as we go to
remove/create dirents instead of having separate additional traversals
to validate the names before we go to modify them.

These checks of the inode should be done up in the operation.  Like
check_ifmt() is done.  We want to see the resources for those inodes and
see them being worked on in the high level operational steps rather than
it being down in the iterators.

> +		ret = ngnfs_inode_get(nfi, &op->txn, NBF_WRITE, dir_ino, &op->dir)		?:
> +		      check_ifmt(op->dir.ninode, S_IFDIR, -ENOTDIR)				?:
> +		      remove_dirent(nfi, &op->txn, &op->dir, &op->da)				?:
> +		      ngnfs_inode_get(nfi, &op->txn, NBF_WRITE, op->da.ino, &op->da.inode)	?:
> +		      ngnfs_inode_update(op->da.inode.tblk, op->da.inode.ninode, -1)		?:
> +		      update_dir(op->dir.tblk, op->dir.ninode, &op->da, -1);

So we'd add those checks on the op->da.inode here after reading it,
rather than reading it again down in remove_dirent.   Could probably
move it up a bit as well so that we work from cheapest to most expensive
amongst the independent operations and checks that could fail.  (The
btree junk is always potentially expensive because it can split/join
blocks and have secondary costs working with allocators.)

> +/*
> + * Update an inode to reflect the addition or removal of a link to it.
> + */
> +int ngnfs_inode_update(struct ngnfs_txn_block *tblk, struct ngnfs_inode *inode, int posneg)
> +{
> +	s32 delta;
> +
> +	delta = posneg * 1;
> +	if ((le32_to_cpu(inode->nlink) + delta >= NGNFS_LINK_MAX))
> +		return -EMLINK;
> +
> +	ngnfs_tblk_assign(tblk, inode->nlink, cpu_to_le32(le32_to_cpu(inode->nlink) + delta));
> +
> +	return 0;
> +}

Ugh, sorry for writing that posneg pattern.  It stinks.  Now that we're
promoting it to a proper function, can we make it look a lot less weird
and just pass in the s32 nlink delta and have it return errors for both
over or underflow?

- z