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

[Valerie Aurora]

On Sat, Mar 1, 2025 at 12:56 AM Zach Brown <zab at zabbo.net> wrote:
>
> 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.

Thanks, fixed! On the side, I'm trying to think of ways to make the
iter() functions a little harder to misuse since this is the second
time I've made a similar mistake... maybe a standard way to indicate
if the iter() function was ever called and returned 0?

> > +     /* 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.

Phew! Yes, that was miscommunication, thanks so much for clearing that up.

> > +             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.)

Agreed! The options for ordering checks from cheap->expensive as I
understand them are:

1. lookup the dirent, read the inode, do the final checks, then remove
the dirent (two btree walks, saves rare btree merges)

2. remove the dirent, read the inode, do the final checks (saves one
btree walk, may merge btree nodes unnecessarily)

>From your experience with scoutfs, you will have a better sense of
what is the right tradeoff (or a better solution).

> > +/*
> > + * 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?

Done! And fixed several bugs while at it, yikes, so thanks for the review!

Valerie