Re: [PATCH 2/3] rust: file: add f_pos and set_f_pos

[Alice Ryhl]

On Fri, Sep 27, 2024 at 9:38 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, Sep 27, 2024 at 08:56:50AM +0200, Alice Ryhl wrote:
>
> > Okay, interesting. I did not know about all of these llseek helpers.
> > I'm definitely happy to make the Rust API force users to do the right
> > thing if we can.
> >
> > It sounds like we basically have a few different seeking behaviors
> > that the driver can choose between, and we want to force the user to
> > use one of them?
>
> Depends...  Basically, SEEK_HOLE/SEEK_DATA is seriously fs-specific
> (unsurprisingly), and pretty much everything wants the usual relation
> between SEEK_SET and SEEK_CUR (<SEEK_CUR,n> is the same as <SEEK_SET,
> current position + n>).  SEEK_END availability varies - the simplest
> variant is <SEEK_END, n> == <SEEK_SET, size + n>, but there are
> cases that genuinely have nothing resembling end-relative seek
> (e.g. anything seq_file-related).
>
> It's not so much available instances as available helpers; details of
> semantics may seriously vary by the driver.
>
> Note that once upon a time ->f_pos had been exposed to ->read() et.al.;
> caused recurring bugs, until we switched to "sample ->f_pos before calling
> ->read(), pass the reference to local copy into the method, then put
> what's the method left behind in there back into ->f_pos".
>
> Something similar might be a good idea for ->llseek().  Locking is
> an unpleasant problem, unfortunately.  lseek() is not a terribly hot
> codepath, but read() and write() are.  For a while we used to do exclusion
> on per-struct file basis for _all_ read/write/lseek; see 797964253d35
> "file: reinstate f_pos locking optimization for regular files" for the
> point where it eroded.
>
> FWIW, I suspect that unconditionally taking ->f_pos_mutex for llseek(2)
> would solve most of the problems - for one thing, with guaranteed
> per-struct-file serialization of vfs_llseek() we could handle SEEK_CUR
> right there, so that ->llseek() instances would never see it; for another,
> we just might be able to pull the same 'pass a reference to local variable
> and let it be handled there' trick for ->llseek().  That would require
> an audit of locking in the instances, though...

Okay, thanks for the explanation. The file position stuff seems pretty
complicated.

One thing to think about is whether there are some behaviors used by
old drivers that new drivers should not use. We can design our Rust
APIs to prevent using it in those legacy ways.

For now I'm dropping this patch from the series at Greg's request.

Alice