On Fri, 2024-08-30 at 15:04 +0200, Christian Brauner wrote: > Probably not for this merge window anymore but let me get it out now: > > The f_version member in struct file isn't particularly well-defined. It > is mainly used as a cookie to detect concurrent seeks when iterating > directories. But it is also abused by some subsystems for completely > unrelated things. > > It is mostly a directory specific thing that doesn't really need to live > in struct file and with its wonky semantics it really lacks a specific > function. > > For pipes, f_version is (ab)used to defer poll notifications until a > write has happened. And struct pipe_inode_info is used by multiple > struct files in their ->private_data so there's no chance of pushing > that down into file->private_data without introducing another pointer > indirection. > > But this should be a solvable problem. Only regular files with > FMODE_ATOMIC_POS and directories require f_pos_lock. Pipes and other > files don't. So this adds a union into struct file encompassing > f_pos_lock and a pipe specific f_pipe member that pipes can use. This > union of course can be extended to other file types and is similar to > what we do in struct inode already. > > I hope I got the details right. It at least holds up in xfstests and LTP > and my hand-rolled getdents-seek stressor. > > > Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx> > --- > Christian Brauner (20): > file: remove pointless comment > adi: remove unused f_version > ceph: remove unused f_version > s390: remove unused f_version > fs: add vfs_setpos_cookie() > fs: add must_set_pos() > fs: use must_set_pos() > fs: add generic_llseek_cookie() > affs: store cookie in private data > ext2: store cookie in private data > ext4: store cookie in private data > input: remove f_version abuse > ocfs2: store cookie in private data > proc: store cookie in private data > udf: store cookie in private data > ufs: store cookie in private data > ubifs: store cookie in private data > fs: add f_pipe > pipe: use f_pipe > fs: remove f_version > > drivers/char/adi.c | 1 - > drivers/input/input.c | 8 +- > drivers/s390/char/hmcdrv_dev.c | 3 - > fs/affs/dir.c | 44 +++++++++-- > fs/ceph/dir.c | 1 - > fs/ext2/dir.c | 28 ++++++- > fs/ext4/dir.c | 50 ++++++------ > fs/ext4/ext4.h | 2 + > fs/ext4/inline.c | 7 +- > fs/file_table.c | 1 - > fs/ocfs2/dir.c | 3 +- > fs/ocfs2/file.c | 11 ++- > fs/ocfs2/file.h | 1 + > fs/pipe.c | 6 +- > fs/proc/base.c | 18 +++-- > fs/read_write.c | 169 +++++++++++++++++++++++++++++++---------- > fs/ubifs/dir.c | 65 +++++++++++----- > fs/udf/dir.c | 28 ++++++- > fs/ufs/dir.c | 28 ++++++- > include/linux/fs.h | 14 +++- > 20 files changed, 366 insertions(+), 122 deletions(-) > --- > base-commit: 79868ff5ce9156228f9aef351d8bf76fb8540230 > change-id: 20240829-vfs-file-f_version-66321e3dafeb > I like it! Fairly straightforward, and it's good to see f_version (which has always been poorly defined) go away. Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
