Re: [PATCH RFC 00/20] file: remove f_version

[Jeff Layton <jlayton@kernel.org>]

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@kernel.org>
> ---
> 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@kernel.org>