On Tue, 13 Sept 2022 at 03:51, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> 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.
Was there a requirement for CIFS supporting tmpfile? Is so where?
>
> 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()...
>
Agreed with all of the above.
>
> 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.
Sure, that would have been the next step after the API is settled.
> 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?
At the moment FUSE is not supported as upper layer, so it's a non-issue.
But I don't see why copy-up can't be fixed to keep the tmp file open.
In fact that's something I also want to do once the interface is
agreed upon.
Thanks,
Miklos
