| Message ID | 20240830-vfs-file-f_version-v1-14-6d3e4816aa7b@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | file: remove f_version | expand |
On Fri, Aug 30, 2024 at 03:04:55PM GMT, Christian Brauner wrote: > Store the cookie to detect concurrent seeks on directories in > file->private_data. > > Signed-off-by: Christian Brauner <brauner@kernel.org> > --- > fs/proc/base.c | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 72a1acd03675..8a8aab6b9801 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -3870,12 +3870,12 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx) > if (!dir_emit_dots(file, ctx)) > return 0; > > - /* f_version caches the tgid value that the last readdir call couldn't > - * return. lseek aka telldir automagically resets f_version to 0. > + /* We cache the tgid value that the last readdir call couldn't > + * return and lseek resets it to 0. > */ > ns = proc_pid_ns(inode->i_sb); > - tid = (int)file->f_version; > - file->f_version = 0; > + tid = (int)(intptr_t)file->private_data; > + file->private_data = NULL; > for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); > task; > task = next_tid(task), ctx->pos++) { > @@ -3890,7 +3890,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx) > proc_task_instantiate, task, NULL)) { > /* returning this tgid failed, save it as the first > * pid for the next readir call */ > - file->f_version = (u64)tid; > + file->private_data = (void *)(intptr_t)tid; > put_task_struct(task); > break; > } > @@ -3915,6 +3915,12 @@ static int proc_task_getattr(struct mnt_idmap *idmap, > return 0; > } > > +static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence) > +{ > + return generic_llseek_cookie(file, offset, whence, > + (u64 *)(uintptr_t)&file->private_data); Btw, this is fixed in-tree (I did send out an unfixed version): static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence) { u64 cookie = 1; loff_t off; off = generic_llseek_cookie(file, offset, whence, &cookie); if (!cookie) file->private_data = NULL; /* serialized by f_pos_lock */ return off; }
On Fri 30-08-24 15:04:55, Christian Brauner wrote: > Store the cookie to detect concurrent seeks on directories in > file->private_data. > > Signed-off-by: Christian Brauner <brauner@kernel.org> ... > ns = proc_pid_ns(inode->i_sb); > - tid = (int)file->f_version; > - file->f_version = 0; > + tid = (int)(intptr_t)file->private_data; What's the point of first casting to intptr_t? > @@ -3890,7 +3890,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx) > proc_task_instantiate, task, NULL)) { > /* returning this tgid failed, save it as the first > * pid for the next readir call */ > - file->f_version = (u64)tid; > + file->private_data = (void *)(intptr_t)tid; The same here... > put_task_struct(task); > break; > } > @@ -3915,6 +3915,12 @@ static int proc_task_getattr(struct mnt_idmap *idmap, > return 0; > } > > +static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence) > +{ > + return generic_llseek_cookie(file, offset, whence, > + (u64 *)(uintptr_t)&file->private_data); So this I think is wrong for 32-bit archs because generic_llseek_cookie() will store 8 bytes there while file->private_data has only 4 bytes... Honza
On Tue 03-09-24 13:34:30, Christian Brauner wrote: > On Fri, Aug 30, 2024 at 03:04:55PM GMT, Christian Brauner wrote: > > Store the cookie to detect concurrent seeks on directories in > > file->private_data. > > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > --- > > fs/proc/base.c | 18 ++++++++++++------ > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > index 72a1acd03675..8a8aab6b9801 100644 > > --- a/fs/proc/base.c > > +++ b/fs/proc/base.c > > @@ -3870,12 +3870,12 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx) > > if (!dir_emit_dots(file, ctx)) > > return 0; > > > > - /* f_version caches the tgid value that the last readdir call couldn't > > - * return. lseek aka telldir automagically resets f_version to 0. > > + /* We cache the tgid value that the last readdir call couldn't > > + * return and lseek resets it to 0. > > */ > > ns = proc_pid_ns(inode->i_sb); > > - tid = (int)file->f_version; > > - file->f_version = 0; > > + tid = (int)(intptr_t)file->private_data; > > + file->private_data = NULL; > > for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); > > task; > > task = next_tid(task), ctx->pos++) { > > @@ -3890,7 +3890,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx) > > proc_task_instantiate, task, NULL)) { > > /* returning this tgid failed, save it as the first > > * pid for the next readir call */ > > - file->f_version = (u64)tid; > > + file->private_data = (void *)(intptr_t)tid; > > put_task_struct(task); > > break; > > } > > @@ -3915,6 +3915,12 @@ static int proc_task_getattr(struct mnt_idmap *idmap, > > return 0; > > } > > > > +static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence) > > +{ > > + return generic_llseek_cookie(file, offset, whence, > > + (u64 *)(uintptr_t)&file->private_data); > > Btw, this is fixed in-tree (I did send out an unfixed version): > > static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence) > { > u64 cookie = 1; > loff_t off; > > off = generic_llseek_cookie(file, offset, whence, &cookie); > if (!cookie) > file->private_data = NULL; /* serialized by f_pos_lock */ > return off; > } Ah, midair collision :). This looks better just why don't you store the cookie unconditionally in file->private_data? This way proc_dir_llseek() makes assumptions about how generic_llseek_cookie() uses the cookie which unnecessarily spreads internal VFS knowledge into filesystems... Honza
On Tue, Sep 03, 2024 at 03:35:48PM GMT, Jan Kara wrote: > On Tue 03-09-24 13:34:30, Christian Brauner wrote: > > On Fri, Aug 30, 2024 at 03:04:55PM GMT, Christian Brauner wrote: > > > Store the cookie to detect concurrent seeks on directories in > > > file->private_data. > > > > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > > --- > > > fs/proc/base.c | 18 ++++++++++++------ > > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > > index 72a1acd03675..8a8aab6b9801 100644 > > > --- a/fs/proc/base.c > > > +++ b/fs/proc/base.c > > > @@ -3870,12 +3870,12 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx) > > > if (!dir_emit_dots(file, ctx)) > > > return 0; > > > > > > - /* f_version caches the tgid value that the last readdir call couldn't > > > - * return. lseek aka telldir automagically resets f_version to 0. > > > + /* We cache the tgid value that the last readdir call couldn't > > > + * return and lseek resets it to 0. > > > */ > > > ns = proc_pid_ns(inode->i_sb); > > > - tid = (int)file->f_version; > > > - file->f_version = 0; > > > + tid = (int)(intptr_t)file->private_data; > > > + file->private_data = NULL; > > > for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); > > > task; > > > task = next_tid(task), ctx->pos++) { > > > @@ -3890,7 +3890,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx) > > > proc_task_instantiate, task, NULL)) { > > > /* returning this tgid failed, save it as the first > > > * pid for the next readir call */ > > > - file->f_version = (u64)tid; > > > + file->private_data = (void *)(intptr_t)tid; > > > put_task_struct(task); > > > break; > > > } > > > @@ -3915,6 +3915,12 @@ static int proc_task_getattr(struct mnt_idmap *idmap, > > > return 0; > > > } > > > > > > +static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence) > > > +{ > > > + return generic_llseek_cookie(file, offset, whence, > > > + (u64 *)(uintptr_t)&file->private_data); > > > > Btw, this is fixed in-tree (I did send out an unfixed version): > > > > static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence) > > { > > u64 cookie = 1; > > loff_t off; > > > > off = generic_llseek_cookie(file, offset, whence, &cookie); > > if (!cookie) > > file->private_data = NULL; /* serialized by f_pos_lock */ > > return off; > > } > > Ah, midair collision :). This looks better just why don't you store the > cookie unconditionally in file->private_data? This way proc_dir_llseek() > makes assumptions about how generic_llseek_cookie() uses the cookie which > unnecessarily spreads internal VFS knowledge into filesystems... I tried to avoid an allocation for procfs (I assume that's what you're getting at). That's basically all.
On Tue 03-09-24 16:00:56, Christian Brauner wrote: > On Tue, Sep 03, 2024 at 03:35:48PM GMT, Jan Kara wrote: > > On Tue 03-09-24 13:34:30, Christian Brauner wrote: > > > On Fri, Aug 30, 2024 at 03:04:55PM GMT, Christian Brauner wrote: > > > > Store the cookie to detect concurrent seeks on directories in > > > > file->private_data. > > > > > > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > > > --- > > > > fs/proc/base.c | 18 ++++++++++++------ > > > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > > > index 72a1acd03675..8a8aab6b9801 100644 > > > > --- a/fs/proc/base.c > > > > +++ b/fs/proc/base.c > > > > @@ -3870,12 +3870,12 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx) > > > > if (!dir_emit_dots(file, ctx)) > > > > return 0; > > > > > > > > - /* f_version caches the tgid value that the last readdir call couldn't > > > > - * return. lseek aka telldir automagically resets f_version to 0. > > > > + /* We cache the tgid value that the last readdir call couldn't > > > > + * return and lseek resets it to 0. > > > > */ > > > > ns = proc_pid_ns(inode->i_sb); > > > > - tid = (int)file->f_version; > > > > - file->f_version = 0; > > > > + tid = (int)(intptr_t)file->private_data; > > > > + file->private_data = NULL; > > > > for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); > > > > task; > > > > task = next_tid(task), ctx->pos++) { > > > > @@ -3890,7 +3890,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx) > > > > proc_task_instantiate, task, NULL)) { > > > > /* returning this tgid failed, save it as the first > > > > * pid for the next readir call */ > > > > - file->f_version = (u64)tid; > > > > + file->private_data = (void *)(intptr_t)tid; > > > > put_task_struct(task); > > > > break; > > > > } > > > > @@ -3915,6 +3915,12 @@ static int proc_task_getattr(struct mnt_idmap *idmap, > > > > return 0; > > > > } > > > > > > > > +static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence) > > > > +{ > > > > + return generic_llseek_cookie(file, offset, whence, > > > > + (u64 *)(uintptr_t)&file->private_data); > > > > > > Btw, this is fixed in-tree (I did send out an unfixed version): > > > > > > static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence) > > > { > > > u64 cookie = 1; > > > loff_t off; > > > > > > off = generic_llseek_cookie(file, offset, whence, &cookie); > > > if (!cookie) > > > file->private_data = NULL; /* serialized by f_pos_lock */ > > > return off; > > > } > > > > Ah, midair collision :). This looks better just why don't you store the > > cookie unconditionally in file->private_data? This way proc_dir_llseek() > > makes assumptions about how generic_llseek_cookie() uses the cookie which > > unnecessarily spreads internal VFS knowledge into filesystems... > > I tried to avoid an allocation for procfs (I assume that's what you're > getting at). That's basically all. Yes, I just meant I'd find it safer to have: static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence) { u64 cookie = (u64)file->private_data; loff_t off; off = generic_llseek_cookie(file, offset, whence, &cookie); file->private_data = (void *)cookie; /* serialized by f_pos_lock */ return off; } So that we don't presume what generic_llseek_cookie() can do with the cookie. Honza
On Wed, Sep 04, 2024 at 04:16:07PM GMT, Jan Kara wrote: > On Tue 03-09-24 16:00:56, Christian Brauner wrote: > > On Tue, Sep 03, 2024 at 03:35:48PM GMT, Jan Kara wrote: > > > On Tue 03-09-24 13:34:30, Christian Brauner wrote: > > > > On Fri, Aug 30, 2024 at 03:04:55PM GMT, Christian Brauner wrote: > > > > > Store the cookie to detect concurrent seeks on directories in > > > > > file->private_data. > > > > > > > > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > > > > --- > > > > > fs/proc/base.c | 18 ++++++++++++------ > > > > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > > > > index 72a1acd03675..8a8aab6b9801 100644 > > > > > --- a/fs/proc/base.c > > > > > +++ b/fs/proc/base.c > > > > > @@ -3870,12 +3870,12 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx) > > > > > if (!dir_emit_dots(file, ctx)) > > > > > return 0; > > > > > > > > > > - /* f_version caches the tgid value that the last readdir call couldn't > > > > > - * return. lseek aka telldir automagically resets f_version to 0. > > > > > + /* We cache the tgid value that the last readdir call couldn't > > > > > + * return and lseek resets it to 0. > > > > > */ > > > > > ns = proc_pid_ns(inode->i_sb); > > > > > - tid = (int)file->f_version; > > > > > - file->f_version = 0; > > > > > + tid = (int)(intptr_t)file->private_data; > > > > > + file->private_data = NULL; > > > > > for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); > > > > > task; > > > > > task = next_tid(task), ctx->pos++) { > > > > > @@ -3890,7 +3890,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx) > > > > > proc_task_instantiate, task, NULL)) { > > > > > /* returning this tgid failed, save it as the first > > > > > * pid for the next readir call */ > > > > > - file->f_version = (u64)tid; > > > > > + file->private_data = (void *)(intptr_t)tid; > > > > > put_task_struct(task); > > > > > break; > > > > > } > > > > > @@ -3915,6 +3915,12 @@ static int proc_task_getattr(struct mnt_idmap *idmap, > > > > > return 0; > > > > > } > > > > > > > > > > +static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence) > > > > > +{ > > > > > + return generic_llseek_cookie(file, offset, whence, > > > > > + (u64 *)(uintptr_t)&file->private_data); > > > > > > > > Btw, this is fixed in-tree (I did send out an unfixed version): > > > > > > > > static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence) > > > > { > > > > u64 cookie = 1; > > > > loff_t off; > > > > > > > > off = generic_llseek_cookie(file, offset, whence, &cookie); > > > > if (!cookie) > > > > file->private_data = NULL; /* serialized by f_pos_lock */ > > > > return off; > > > > } > > > > > > Ah, midair collision :). This looks better just why don't you store the > > > cookie unconditionally in file->private_data? This way proc_dir_llseek() > > > makes assumptions about how generic_llseek_cookie() uses the cookie which > > > unnecessarily spreads internal VFS knowledge into filesystems... > > > > I tried to avoid an allocation for procfs (I assume that's what you're > > getting at). That's basically all. > > Yes, I just meant I'd find it safer to have: > > static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence) > { > u64 cookie = (u64)file->private_data; > loff_t off; > > off = generic_llseek_cookie(file, offset, whence, &cookie); > file->private_data = (void *)cookie; /* serialized by f_pos_lock */ > return off; > } > > So that we don't presume what generic_llseek_cookie() can do with the > cookie. I switched to that, thanks!
diff --git a/fs/proc/base.c b/fs/proc/base.c index 72a1acd03675..8a8aab6b9801 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -3870,12 +3870,12 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx) if (!dir_emit_dots(file, ctx)) return 0; - /* f_version caches the tgid value that the last readdir call couldn't - * return. lseek aka telldir automagically resets f_version to 0. + /* We cache the tgid value that the last readdir call couldn't + * return and lseek resets it to 0. */ ns = proc_pid_ns(inode->i_sb); - tid = (int)file->f_version; - file->f_version = 0; + tid = (int)(intptr_t)file->private_data; + file->private_data = NULL; for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns); task; task = next_tid(task), ctx->pos++) { @@ -3890,7 +3890,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx) proc_task_instantiate, task, NULL)) { /* returning this tgid failed, save it as the first * pid for the next readir call */ - file->f_version = (u64)tid; + file->private_data = (void *)(intptr_t)tid; put_task_struct(task); break; } @@ -3915,6 +3915,12 @@ static int proc_task_getattr(struct mnt_idmap *idmap, return 0; } +static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence) +{ + return generic_llseek_cookie(file, offset, whence, + (u64 *)(uintptr_t)&file->private_data); +} + static const struct inode_operations proc_task_inode_operations = { .lookup = proc_task_lookup, .getattr = proc_task_getattr, @@ -3925,7 +3931,7 @@ static const struct inode_operations proc_task_inode_operations = { static const struct file_operations proc_task_operations = { .read = generic_read_dir, .iterate_shared = proc_task_readdir, - .llseek = generic_file_llseek, + .llseek = proc_dir_llseek, }; void __init set_proc_pid_nlink(void)
Store the cookie to detect concurrent seeks on directories in file->private_data. Signed-off-by: Christian Brauner <brauner@kernel.org> --- fs/proc/base.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)