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