Re: [PATCH 2/2] smb: client: add support for O_TMPFILE

[Paulo Alcantara <pc@manguebit.org>]

NeilBrown <neilb@ownmail.net> writes:

> On Mon, 06 Apr 2026, Al Viro wrote:
>> On Sun, Apr 05, 2026 at 06:18:19PM -0300, Paulo Alcantara wrote:
>> 
>> > @@ -436,17 +457,13 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
>> >  		goto out_err;
>> >  	}
>> >  
>> > -	if (newinode)
>> > -		if (S_ISDIR(newinode->i_mode)) {
>> > -			rc = -EISDIR;
>> > -			goto out_err;
>> > -		}
>> > +	if (newinode && S_ISDIR(newinode->i_mode)) {
>> > +		rc = -EISDIR;
>> > +		goto out_err;
>> > +	}
>> >  
>> >  	d_drop(direntry);
>> > -	d_add(direntry, newinode);
>> 
>> > +		rc = __cifs_do_create(dir, direntry, full_path, xid,
>> > +				      tlink, oflags, mode, oplock,
>> > +				      fid, buf, &inode);
>> > +		if (!rc)
>> > +			d_add(direntry, inode);
>> 
>> 
>> > +		rc = __cifs_do_create(dir, dentry, path, xid, tlink,
>> > +				      file->f_flags, mode, &oplock,
>> > +				      &fid, NULL, &inode);
>> > +		if (!rc) {
>> > +			set_nlink(inode, 0);
>> > +			mark_inode_dirty(inode);
>> > +			d_mark_tmpfile_name(file, &QSTR_LEN(name, size - 1));
>> > +			d_instantiate(dentry, inode);
>> 
>> I really don't like this "not sure what the state is, d_drop() to
>> get it unhashed" pattern, _especially_ when d_drop() and d_add() or
>> d_instantiate() get separated.
>> 
>> Note, BTW, this d_add() call site is one of the two in the entire kernel
>> that are neither d_splice_alias() in disguise nor pass NULL as inode.
>> The other one is nfs_link(), where we also have this kind of "d_drop()
>> first, then use d_add()" pattern.
>> 
>> Folks, what state can dentry be in cifs_do_create()?  I'd rather see
>> that sorted out, not obfuscated even more.
>
> cifs_do_create() gets call from cifs_atomic_open() which is
> ->atomic_open(), so dentry can be in-lookup or negative-hashed.
>
> If is also called from cifs_create() (->create()) and as cifs_lookup()
> never skips the lookup due to intent (like NFS does) the dentry will
> always be hashed negative.

Thanks Neil for the details!

IIUC, this is what we currently have

* cifs_atomic_open()

  init state: in-lookup or hashed-negative
  [d_drop() + d_add()]
  end state: hashed-positive

* cifs_tmpfile()

  init state: unhashed-negative
  [d_drop() + d_instantiate()]
  end state: unhashed-positive

* cifs_create()

  init state: hashed-negative
  [d_drop() + d_add()]
  end state: hashed-positive