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

[Al Viro]

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...