On Sat, 08 Feb 2025, Al Viro wrote: > On Thu, Feb 06, 2025 at 04:42:38PM +1100, NeilBrown wrote: > > vfs_mkdir() does not guarantee to make the child dentry positive on > > success. It may leave it negative and then the caller needs to perform a > > lookup to find the target dentry. > > > > This patch introduced vfs_mkdir_return() which performs the lookup if > > needed so that this code is centralised. > > > > This prepares for a new inode operation which will perform mkdir and > > returns the correct dentry. > > * Calling conventions stink; make it _consume_ dentry reference and > return dentry reference or ERR_PTR(). Callers will be happier that way > (check it). With later patches it will need to consume the lock on the dentry as well, and either transfer it to the new one or (for error) unlock it. We need to have the result dentry still locked for fsnotify_mkdir(). Transferring the dentry lock would have to be done in d_splice_alias(). The __d_unalias() branch should be ok because I already trylock in there and fail if I can't get the lock. For the IS_ROOT branch .... I think it is safe to fail if a trylock doesn't succeed. So I can probably make that work - thanks. Hmm... kernfs reportedly can leave the mkdir dentry negative and fill in the inode later. How does that work? I assume it will still be hashed so mkdir won't try the lookup. done_path_create() will need to accept an IS_ERR() dentry. > > * Calling conventions should be documented in commit message *and* in > D/f/porting What is the scope of "porting" ? IT seems to be mostly about _operations interfaces, but I do see other things in there. I'll try to remember that - thanks. > > * devpts, nfs4recover and xfs might as well convert (not going to hit > the "need a lookup" case anyway) good point - avoiding the lookup when not requested is a pointless optimisation because it is hardly every needed and should always be cheap - we expect it to be in the dcache. > > * that > + /* Need a "const" pointer. We know d_name is const > + * because we hold an exclusive lock on i_rwsem > + * in d_parent. > + */ > + const struct qstr *d_name = (void*)&dentry->d_name; > + d = lookup_dcache(d_name, dentry->d_parent, 0); > + if (!d) > + d = __lookup_slow(d_name, dentry->d_parent, 0); > doesn't need a cast. C is perfectly fine with > T *x = foo(); > const T *y = x; > > You are not allowed to _strip_ qualifiers; adding them is fine. > Same reason why you are allowed to pass char * to strlen() without > any casts whatsoever. hmm.. I thought I had tried that. Maybe I didn't try hard enough. Thanks for the guidance. > > Comment re stability is fine; the cast is pure WTF material. > Thanks, NeilBrown
