Re: [LSF/MM/BPF TOPIC] Allowing linkat() to replace the destination

[Omar Sandoval <osandov@osandov.com>]

On Wed, Jan 22, 2020 at 08:57:01AM +0200, Amir Goldstein wrote:
> On Wed, Jan 22, 2020 at 1:05 AM Omar Sandoval <osandov@osandov.com> wrote:
> >
> > On Sat, Jan 18, 2020 at 02:20:32AM +0000, Al Viro wrote:
> > > On Fri, Jan 17, 2020 at 05:17:34PM -0800, Omar Sandoval wrote:
> > > > > No.  This is completely wrong; just make it ->link_replace() and be done
> > > > > with that; no extra arguments and *always* the same conditions wrt
> > > > > positive/negative.  One of the reasons why ->rename() tends to be
> > > > > ugly (and a source of quite a few bugs over years) are those "if
> > > > > target is positive/if target is negative" scattered over the instances.
> > > > >
> > > > > Make the choice conditional upon the positivity of target.
> > > >
> > > > Yup, you already convinced me that ->link_replace() is better in your
> > > > last email.
> > >
> > > FWIW, that might be not so simple ;-/  Reason: NFS-like stuff.  Client
> > > sees a negative in cache; the problem is how to decide whether to
> > > tell the server "OK, I want normal link()" vs. "if it turns out that
> > > someone has created it by the time you see the request, give do
> > > a replacing link".  Sure, if could treat ->link() telling you -EEXIST
> > > as "OK, repeat it with ->link_replace(), then", but that's an extra
> > > roundtrip...
> >
> > So that's a point in favor of ->link(). But then if we overload ->link()
> > instead of adding ->link_replace() and we want EOPNOTSUPP to fail fast,
> > we need to add something like FMODE_SUPPORTS_AT_REPLACE.
> >
> > Some options I see are:
> >
> > 1. Go with ->link_replace() until network filesystem specs support
> >    AT_REPLACE. That would be a bit of a mess down the line, though.
> > 2. Stick with ->link(), let the filesystem implementations deal with the
> >    positive targets, and add FMODE_SUPPORTS_AT_REPLACE so that feature
> >    detection remains easy for userspace.
> 
> "detection remains easy..." why is this important?

As I mentioned, I don't think it's necessary given the precedent.
However, Al voiced some concern over this earlier in the thread:

---
> > >        7) how do users tell if filesystem supports that?  And no,
> > >references to pathconf, Cthulhu and other equally delightful entities
> > >are not really welcome.
> >
> > EOPNOTSUPP is probably the most helpful.
> 
> Umm...  What would you feed it, though?  You need to get past your
> "links to the same file, do nothing" escape...
---

> Do you know of a userspace application that would have a problem checking
> if AT_REPLACE works, fall back to whatever and never try it ever again?
> 
> Besides, when said application tried to open an O_TMPFILE and fail, it
> will have already detected a lot of the unsupported cases.
> Sorry for not reading all the thread again, some API questions:
> - We intend to allow AT_REPLACE only with O_TMPFILE src. Right?

I wasn't planning on having that restriction. It's not too much effort
for filesystems to support it for normal files, so I wouldn't want to
place an artificial restriction on a useful primitive.

> - Does AT_REPLACE assert that destination is positive? and if so why?

No, it should work like a normal link() if the destination doesn't
exist.

> The functionality that is complement to atomic rename would be atomic
> link, destination could be positive or negative, but end results will be
> that destination is positive with new inode.
> With those semantics, ->link_replace() makes much less sense IMO.
> 
> > 3. Option 2, but don't bother with FMODE_SUPPORTS_AT_REPLACE.
> >
> > FWIW, there is precendent for option 3: RENAME_EXCHANGE. That has the
> > same "files are the same" noop condition, and we don't know whether
> > RENAME_EXCHANGE is supported until ->rename(). A cursory search shows
> > that applications using RENAME_EXCHANGE try it and fall back to a
> > non-atomic exchange on EINVAL. They could do the exact same thing for
> > AT_REPLACE.
> >
> 
> That sounds like the most reasonable approach to me. Let's not over complicate.
> If you find that creates too much generic logic in ->link(), you can take
> the approach Darrick employed with generic_remap_file_range_prep() for
> filesystems that want to support AT_REPLACE. All other fs just need to check
> for valid flags mask, like the ->rename() precedent.
> 
> Another side discussion about passing AT_ flags down to filesystems.
> Traditionally, that was never done, until AT_STATX_ mixed vfs flags
> with filesystem flags on the same AT_ namespace.
> Now we have linkat() syscall that can take only AT_ vfs flags and
> renameat2() syscall that can take only RENAME_ filesystem flags not
> from the AT_ namespace.
> I feel that the situation of having AT_REPLACE API along with
> RENAME_EXCHANGE and RENAME_NOREPLACE is a bit awkward
> and some standardization is in order.
> 
> According to include/uapi/linux/fcntl.h, there is no numeric collision
> between the RENAME_ flag namepsace and AT_ flags namespace,
> although I do find it suspicious that AT_ flags start at 0x100...
> Could we define AT_RENAME_xxx RENAME_xxx flags and name the
> new flag AT_LINK_REPLACE, so it is a bit more clear that the flag
> is specific to link(2) syscall and not vfs generic AT_ flag.

Sure, I'll rename it to AT_LINK_REPLACE. AT_RENAME_xxx is probably for a
separate series.