Re: [PATCH 0/3} Change ->mkdir() and vfs_mkdir() to return a dentry

NeilBrown
2025-02-17T03:54:00Z

On Fri, 14 Feb 2025, Al Viro wrote:
> On Fri, Feb 14, 2025 at 04:16:40PM +1100, NeilBrown wrote:
> > This is a small set of patches which are needed before we can make the
> > locking on directory operations more fine grained. I think they are
> > useful even if we don't go that direction.
> >
> > Some callers of vfs_mkdir() need to operation on the resulting directory
> > but cannot be guaranteed that the dentry will be hashed and positive on
> > success - another dentry might have been used.
> >
> > This patch changes ->mkdir to return a dentry, changes NFS in particular
> > to return the correct dentry (I believe it is the only filesystem to
> > possibly not use the given dentry), and changes vfs_mkdir() to return
> > that dentry, removing the look that a few callers currently need.
> >
> > I have not Cc: the developers of all the individual filesystems - only
> > NFS. I have build-tested all the changes except hostfs. I can email
> > them explicitly if/when this is otherwise acceptable. If anyone sees
> > this on fs-devel and wants to provide a pre-emptive ack I will collect
> > those and avoid further posting for those fs.
>
> 1) please, don't sprinkle the PTR_ERR_OR_ZERO() shite all over the place.
> Almost always the same thing can be done without it and it ends up
> being cleaner. Seriously.

I've removed several PTR_ERR_OR_ZERO() calls. Some times that could be
seen as a slight improvement, other times possibly a slight negative
(depending on how one feels about PTR_ERR_OR_ZERO of course). I have
left three as I cannot see how to remove them without making the code
significant more clumsy. If you find the remaining few to still be
objectionable I would be happy to see what alternate you would propose.

Your other feedback has been quite helpful - thanks.

NeilBrown

>
> 2) I suspect that having method instances return NULL for "just use the
> argument" would would be harder to fuck up; basically, the same as for
> ->lookup() instances. I'll try to tweak it and see what falls out...
>
> 3) I'm pretty sure that NFS is *not* the only filesystem that returns
> unhashed negative in some success cases; will need to go over the instances
> to verify that, though.
>