On Mon, Sep 05, 2022 at 05:51:38PM +0200, Miklos Szeredi wrote:
> On Wed, Aug 31, 2022 at 02:30:40PM -0700, Yu-li Lin wrote:
> > Thanks for the reference. IIUC, the consensus is to make it atomic,
> > although there's no agreement on how it should be done. Does that mean
> > we should hold off on
> > this patch until atomic temp files are figured out higher in the stack
> > or do you have thoughts on how the fuse uapi should look like prior to
> > the vfs/refactoring decision?
>
> Here's a patch refactoring the tmpfile kapi to return an open file instead of a
> dentry.
>
> Comments?
First and foremost: how many operations the interested filesystems (cifs,
for one) are capable of for such beasts? I can believe that FUSE can handle
that, but can something like cifs do it? Their protocol is pathname-based, IIRC;
can they really handle that kind of files without something like sillyrename?
Because if sillyrename and its analogues are in the game, we have seriously
changed rules re directory locking, and we'd better figure that out first.
IOW, I would really like to see proposed instances for FUSE and CIFS - the shape
of the series seriously depends upon that. Before we commit to calling conventions
changes.
That aside, some notes on the patch:
* file->f_path.dentry all over the place is ugly and wrong. If nothing else,
d_tmpfile() could be converted to taking struct file * - nothing outside of
->tmpfile() instances is using it.
* finish_tmpfile() parts would be less noisy if it was finish_tmpfile(file, errror),
starting with if (error) return error;
* a bit of weirdness in ext4:
> err = dquot_initialize(dir);
> if (err)
> - return err;
> + goto out;
Huh? Your out: starts with if (err) return err; Sure, compiler
will figure it out, but what have human readers done to you?
* similar in shmem:
> +out:
> + if (error)
> + return error;
> +
> + return finish_tmpfile(file);
> out_iput:
> iput(inode);
> - return error;
> + goto out;
How the hell would it ever get to out_iput: with error == 0?
* in your vfs_tmpfile() wrapper
> + child = ERR_CAST(file);
> + if (!IS_ERR(file)) {
> + error = vfs_tmpfile_new(mnt_userns, path, file, mode);
> + child = error ? ERR_PTR(error) : dget(file->f_path.dentry);
> + fput(file);
> + }
> + return child;
This really ought to be
if (IS_ERR(file))
return ERR_CAST(file);
...
IS_ERR() comes with implicit unlikely()...
Next, there are users outside of do_o_tmpfile(). The one in cachefiles
is a prime candidate for combining with open - the stuff done between
vfs_tmpfile() and opening the sucker consists of
* cachefiles_ondemand_init_object() - takes no cleanup on
later failure exits, doesn't know anything about dentry created. Might
as well be done before vfs_tmpfile().
* cachefiles_mark_inode_in_use() - which really can't fail there,
and the only thing being done is marking the sucker with S_KERNEL_FILE.
Could be done after opening just as well.
* vfs_truncate() used to expand to required size. Trivially done
after opening, and we probably want struct file there - there's a reason
why ftruncate(2) sets ATTR_FILE/ia_file... We are unlikely to use
anything like FUSE for cachefiles, but leaving traps like that is a bad
idea.
IOW, cachefiles probably wants a preliminary patch series that would
massage it to the "tmpfile right next to open, the use struct file"
form.
Another user is overlayfs, and that's going to get painful. It checks for
->tmpfile() working and if it does work, we use it for copyup. The trouble
is, will e.g. FUSE be ready to handle things like setxattr on such dentries?
It's not just a matter of copying the data - there we would be quite fine
with opened file; it's somewhat clumsy since copyup is done for FIFOs, etc.,
but that could be dealt with. But things like setting metadata are done
by dentry there. And with your patch the file will have been closed by
the time we get to that part. Can e.g. FUSE deal with that?
