[PATCH] fs: don't needlessly acquire f_lock

[PATCH] fs: don't needlessly acquire f_lock

[Christian Brauner]

Before 2011 there was no meaningful synchronization between
read/readdir/write/seek. Only in commit
ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
synchronization was added for SEEK_CUR by taking f_lock around
vfs_setpos().

Then in 2014 full synchronization between read/readdir/write/seek was
added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
for directories. At that point taking f_lock became unnecessary for such
files.

So only acquire f_lock for SEEK_CUR if this isn't a file that would have
acquired f_pos_lock if necessary.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/read_write.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index a6133241dfb8..816189f9c56d 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -168,13 +168,23 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence,
 		return offset;
 
 	if (whence == SEEK_CUR) {
+		bool locked;
+
 		/*
-		 * f_lock protects against read/modify/write race with
-		 * other SEEK_CURs. Note that parallel writes and reads
-		 * behave like SEEK_SET.
+		 * If the file requires locking via f_pos_lock we know
+		 * that mutual exclusion for SEEK_CUR on the same file
+		 * is guaranteed. If the file isn't locked, we take
+		 * f_lock to protect against f_pos races with other
+		 * SEEK_CURs.
 		 */
-		guard(spinlock)(&file->f_lock);
-		return vfs_setpos(file, file->f_pos + offset, maxsize);
+		locked = (file->f_mode & FMODE_ATOMIC_POS) ||
+			 file->f_op->iterate_shared;
+		if (!locked)
+			spin_lock(&file->f_lock);
+		offset = vfs_setpos(file, file->f_pos + offset, maxsize);
+		if (!locked)
+			spin_unlock(&file->f_lock);
+		return offset;
 	}
 
 	return vfs_setpos(file, offset, maxsize);
-- 
2.47.2

Re: [PATCH] fs: don't needlessly acquire f_lock

[Matthew Wilcox]

On Fri, Feb 07, 2025 at 03:10:33PM +0100, Christian Brauner wrote:
> Before 2011 there was no meaningful synchronization between
> read/readdir/write/seek. Only in commit
> ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
> synchronization was added for SEEK_CUR by taking f_lock around
> vfs_setpos().
> 
> Then in 2014 full synchronization between read/readdir/write/seek was
> added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
> by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
> for directories. At that point taking f_lock became unnecessary for such
> files.
> 
> So only acquire f_lock for SEEK_CUR if this isn't a file that would have
> acquired f_pos_lock if necessary.

What workloads benefit from this optimisation?

Re: [PATCH] fs: don't needlessly acquire f_lock

[Jan Kara]

On Fri 07-02-25 15:10:33, Christian Brauner wrote:
> Before 2011 there was no meaningful synchronization between
> read/readdir/write/seek. Only in commit
> ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
> synchronization was added for SEEK_CUR by taking f_lock around
> vfs_setpos().
> 
> Then in 2014 full synchronization between read/readdir/write/seek was
> added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
> by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
> for directories. At that point taking f_lock became unnecessary for such
> files.
> 
> So only acquire f_lock for SEEK_CUR if this isn't a file that would have
> acquired f_pos_lock if necessary.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

...

>  	if (whence == SEEK_CUR) {
> +		bool locked;
> +
>  		/*
> -		 * f_lock protects against read/modify/write race with
> -		 * other SEEK_CURs. Note that parallel writes and reads
> -		 * behave like SEEK_SET.
> +		 * If the file requires locking via f_pos_lock we know
> +		 * that mutual exclusion for SEEK_CUR on the same file
> +		 * is guaranteed. If the file isn't locked, we take
> +		 * f_lock to protect against f_pos races with other
> +		 * SEEK_CURs.
>  		 */
> -		guard(spinlock)(&file->f_lock);
> -		return vfs_setpos(file, file->f_pos + offset, maxsize);
> +		locked = (file->f_mode & FMODE_ATOMIC_POS) ||
> +			 file->f_op->iterate_shared;

As far as I understand the rationale this should match to
file_needs_f_pos_lock() (or it can possibly be weaker) but it isn't obvious
to me that's the case. After thinking about possibilities, I could convince
myself that what you suggest is indeed safe but the condition being in two
completely independent places and leading to subtle bugs if it gets out of
sync seems a bit fragile to me.

								Honza

> +		if (!locked)
> +			spin_lock(&file->f_lock);
> +		offset = vfs_setpos(file, file->f_pos + offset, maxsize);
> +		if (!locked)
> +			spin_unlock(&file->f_lock);
> +		return offset;
>  	}
>  
>  	return vfs_setpos(file, offset, maxsize);
> -- 
> 2.47.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: don't needlessly acquire f_lock

[Mateusz Guzik]

On Fri, Feb 7, 2025 at 4:50 PM Jan Kara <jack@suse.cz> wrote:
>
> On Fri 07-02-25 15:10:33, Christian Brauner wrote:
> > Before 2011 there was no meaningful synchronization between
> > read/readdir/write/seek. Only in commit
> > ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
> > synchronization was added for SEEK_CUR by taking f_lock around
> > vfs_setpos().
> >
> > Then in 2014 full synchronization between read/readdir/write/seek was
> > added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
> > by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
> > for directories. At that point taking f_lock became unnecessary for such
> > files.
> >
> > So only acquire f_lock for SEEK_CUR if this isn't a file that would have
> > acquired f_pos_lock if necessary.
> >
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
>
> ...
>
> >       if (whence == SEEK_CUR) {
> > +             bool locked;
> > +
> >               /*
> > -              * f_lock protects against read/modify/write race with
> > -              * other SEEK_CURs. Note that parallel writes and reads
> > -              * behave like SEEK_SET.
> > +              * If the file requires locking via f_pos_lock we know
> > +              * that mutual exclusion for SEEK_CUR on the same file
> > +              * is guaranteed. If the file isn't locked, we take
> > +              * f_lock to protect against f_pos races with other
> > +              * SEEK_CURs.
> >                */
> > -             guard(spinlock)(&file->f_lock);
> > -             return vfs_setpos(file, file->f_pos + offset, maxsize);
> > +             locked = (file->f_mode & FMODE_ATOMIC_POS) ||
> > +                      file->f_op->iterate_shared;
>
> As far as I understand the rationale this should match to
> file_needs_f_pos_lock() (or it can possibly be weaker) but it isn't obvious
> to me that's the case. After thinking about possibilities, I could convince
> myself that what you suggest is indeed safe but the condition being in two
> completely independent places and leading to subtle bugs if it gets out of
> sync seems a bit fragile to me.
>

A debug-only assert that the lock is held when expected should sort it out?

> > +             if (!locked)
> > +                     spin_lock(&file->f_lock);
> > +             offset = vfs_setpos(file, file->f_pos + offset, maxsize);
> > +             if (!locked)
> > +                     spin_unlock(&file->f_lock);
> > +             return offset;
> >       }
> >
> >       return vfs_setpos(file, offset, maxsize);

btw I ran this sucker over a kernel build:

bpftrace -e 'kprobe:generic_file_llseek_size { @[((struct file
*)arg0)->f_mode & (1 << 15), ((struct file
*)arg0)->f_op->iterate_shared, arg2] = count(); }
'
Attaching 1 probe...
^C

@[32768, 0x0, 2]: 9
@[32768, 0x0, 1]: 171797
@[32768, 0x0, 0]: 660866

SEEK_CUR is 1, so this *does* show up.

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] fs: don't needlessly acquire f_lock

[Christian Brauner]

On Fri, Feb 07, 2025 at 05:42:17PM +0100, Mateusz Guzik wrote:
> On Fri, Feb 7, 2025 at 4:50 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Fri 07-02-25 15:10:33, Christian Brauner wrote:
> > > Before 2011 there was no meaningful synchronization between
> > > read/readdir/write/seek. Only in commit
> > > ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
> > > synchronization was added for SEEK_CUR by taking f_lock around
> > > vfs_setpos().
> > >
> > > Then in 2014 full synchronization between read/readdir/write/seek was
> > > added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
> > > by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
> > > for directories. At that point taking f_lock became unnecessary for such
> > > files.
> > >
> > > So only acquire f_lock for SEEK_CUR if this isn't a file that would have
> > > acquired f_pos_lock if necessary.
> > >
> > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> >
> > ...
> >
> > >       if (whence == SEEK_CUR) {
> > > +             bool locked;
> > > +
> > >               /*
> > > -              * f_lock protects against read/modify/write race with
> > > -              * other SEEK_CURs. Note that parallel writes and reads
> > > -              * behave like SEEK_SET.
> > > +              * If the file requires locking via f_pos_lock we know
> > > +              * that mutual exclusion for SEEK_CUR on the same file
> > > +              * is guaranteed. If the file isn't locked, we take
> > > +              * f_lock to protect against f_pos races with other
> > > +              * SEEK_CURs.
> > >                */
> > > -             guard(spinlock)(&file->f_lock);
> > > -             return vfs_setpos(file, file->f_pos + offset, maxsize);
> > > +             locked = (file->f_mode & FMODE_ATOMIC_POS) ||
> > > +                      file->f_op->iterate_shared;
> >
> > As far as I understand the rationale this should match to
> > file_needs_f_pos_lock() (or it can possibly be weaker) but it isn't obvious
> > to me that's the case. After thinking about possibilities, I could convince
> > myself that what you suggest is indeed safe but the condition being in two
> > completely independent places and leading to subtle bugs if it gets out of
> > sync seems a bit fragile to me.
> >
> 
> A debug-only assert that the lock is held when expected should sort it out?

Good idea. Let me reuse the newly added infra:
VFS_WARN_ON_ONCE(locked && !mutex_is_locked(&file->f_pos_lock));

> 
> > > +             if (!locked)
> > > +                     spin_lock(&file->f_lock);
> > > +             offset = vfs_setpos(file, file->f_pos + offset, maxsize);
> > > +             if (!locked)
> > > +                     spin_unlock(&file->f_lock);
> > > +             return offset;
> > >       }
> > >
> > >       return vfs_setpos(file, offset, maxsize);
> 
> btw I ran this sucker over a kernel build:
> 
> bpftrace -e 'kprobe:generic_file_llseek_size { @[((struct file
> *)arg0)->f_mode & (1 << 15), ((struct file
> *)arg0)->f_op->iterate_shared, arg2] = count(); }
> '
> Attaching 1 probe...
> ^C
> 
> @[32768, 0x0, 2]: 9
> @[32768, 0x0, 1]: 171797
> @[32768, 0x0, 0]: 660866
> 
> SEEK_CUR is 1, so this *does* show up.

Thanks for the perf.

Re: [PATCH] fs: don't needlessly acquire f_lock

[Jan Kara]

On Mon 10-02-25 13:01:38, Christian Brauner wrote:
> On Fri, Feb 07, 2025 at 05:42:17PM +0100, Mateusz Guzik wrote:
> > On Fri, Feb 7, 2025 at 4:50 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Fri 07-02-25 15:10:33, Christian Brauner wrote:
> > > > Before 2011 there was no meaningful synchronization between
> > > > read/readdir/write/seek. Only in commit
> > > > ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
> > > > synchronization was added for SEEK_CUR by taking f_lock around
> > > > vfs_setpos().
> > > >
> > > > Then in 2014 full synchronization between read/readdir/write/seek was
> > > > added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
> > > > by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
> > > > for directories. At that point taking f_lock became unnecessary for such
> > > > files.
> > > >
> > > > So only acquire f_lock for SEEK_CUR if this isn't a file that would have
> > > > acquired f_pos_lock if necessary.
> > > >
> > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > >
> > > ...
> > >
> > > >       if (whence == SEEK_CUR) {
> > > > +             bool locked;
> > > > +
> > > >               /*
> > > > -              * f_lock protects against read/modify/write race with
> > > > -              * other SEEK_CURs. Note that parallel writes and reads
> > > > -              * behave like SEEK_SET.
> > > > +              * If the file requires locking via f_pos_lock we know
> > > > +              * that mutual exclusion for SEEK_CUR on the same file
> > > > +              * is guaranteed. If the file isn't locked, we take
> > > > +              * f_lock to protect against f_pos races with other
> > > > +              * SEEK_CURs.
> > > >                */
> > > > -             guard(spinlock)(&file->f_lock);
> > > > -             return vfs_setpos(file, file->f_pos + offset, maxsize);
> > > > +             locked = (file->f_mode & FMODE_ATOMIC_POS) ||
> > > > +                      file->f_op->iterate_shared;
> > >
> > > As far as I understand the rationale this should match to
> > > file_needs_f_pos_lock() (or it can possibly be weaker) but it isn't obvious
> > > to me that's the case. After thinking about possibilities, I could convince
> > > myself that what you suggest is indeed safe but the condition being in two
> > > completely independent places and leading to subtle bugs if it gets out of
> > > sync seems a bit fragile to me.
> > >
> > 
> > A debug-only assert that the lock is held when expected should sort it out?
> 
> Good idea. Let me reuse the newly added infra:
> VFS_WARN_ON_ONCE(locked && !mutex_is_locked(&file->f_pos_lock));

Fine, but won't this actually trigger? file_needs_f_pos_lock() is:

        return (file->f_mode & FMODE_ATOMIC_POS) &&
                (file_count(file) > 1 || file->f_op->iterate_shared);

and here you have:

        locked = (file->f_mode & FMODE_ATOMIC_POS) ||
                  file->f_op->iterate_shared;

So if (file->f_mode & FMODE_ATOMIC_POS) and (file_count(file) == 1),
file_needs_f_pos_lock() returns false but locked is true?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: don't needlessly acquire f_lock

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 3485 bytes --]

On Mon, Feb 10, 2025 at 03:58:17PM +0100, Jan Kara wrote:
> On Mon 10-02-25 13:01:38, Christian Brauner wrote:
> > On Fri, Feb 07, 2025 at 05:42:17PM +0100, Mateusz Guzik wrote:
> > > On Fri, Feb 7, 2025 at 4:50 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > On Fri 07-02-25 15:10:33, Christian Brauner wrote:
> > > > > Before 2011 there was no meaningful synchronization between
> > > > > read/readdir/write/seek. Only in commit
> > > > > ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
> > > > > synchronization was added for SEEK_CUR by taking f_lock around
> > > > > vfs_setpos().
> > > > >
> > > > > Then in 2014 full synchronization between read/readdir/write/seek was
> > > > > added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
> > > > > by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
> > > > > for directories. At that point taking f_lock became unnecessary for such
> > > > > files.
> > > > >
> > > > > So only acquire f_lock for SEEK_CUR if this isn't a file that would have
> > > > > acquired f_pos_lock if necessary.
> > > > >
> > > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > >
> > > > ...
> > > >
> > > > >       if (whence == SEEK_CUR) {
> > > > > +             bool locked;
> > > > > +
> > > > >               /*
> > > > > -              * f_lock protects against read/modify/write race with
> > > > > -              * other SEEK_CURs. Note that parallel writes and reads
> > > > > -              * behave like SEEK_SET.
> > > > > +              * If the file requires locking via f_pos_lock we know
> > > > > +              * that mutual exclusion for SEEK_CUR on the same file
> > > > > +              * is guaranteed. If the file isn't locked, we take
> > > > > +              * f_lock to protect against f_pos races with other
> > > > > +              * SEEK_CURs.
> > > > >                */
> > > > > -             guard(spinlock)(&file->f_lock);
> > > > > -             return vfs_setpos(file, file->f_pos + offset, maxsize);
> > > > > +             locked = (file->f_mode & FMODE_ATOMIC_POS) ||
> > > > > +                      file->f_op->iterate_shared;
> > > >
> > > > As far as I understand the rationale this should match to
> > > > file_needs_f_pos_lock() (or it can possibly be weaker) but it isn't obvious
> > > > to me that's the case. After thinking about possibilities, I could convince
> > > > myself that what you suggest is indeed safe but the condition being in two
> > > > completely independent places and leading to subtle bugs if it gets out of
> > > > sync seems a bit fragile to me.
> > > >
> > > 
> > > A debug-only assert that the lock is held when expected should sort it out?
> > 
> > Good idea. Let me reuse the newly added infra:
> > VFS_WARN_ON_ONCE(locked && !mutex_is_locked(&file->f_pos_lock));
> 
> Fine, but won't this actually trigger? file_needs_f_pos_lock() is:
> 
>         return (file->f_mode & FMODE_ATOMIC_POS) &&
>                 (file_count(file) > 1 || file->f_op->iterate_shared);
> 
> and here you have:
> 
>         locked = (file->f_mode & FMODE_ATOMIC_POS) ||
>                   file->f_op->iterate_shared;
> 
> So if (file->f_mode & FMODE_ATOMIC_POS) and (file_count(file) == 1),
> file_needs_f_pos_lock() returns false but locked is true?

Sorry, I got lost in other mails.
Yes, you're right. I had changed the patch in my tree to fix that.
I'll append it here. Sorry about that. Tell me if that looks sane ok to
you now.

[-- Attachment #2: 0001-fs-don-t-needlessly-acquire-f_lock.patch --]
[-- Type: text/x-diff, Size: 2965 bytes --]

From 1534e4816a5c128dbf6e6942ab43b780ec51b336 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Fri, 7 Feb 2025 15:10:33 +0100
Subject: [PATCH] fs: don't needlessly acquire f_lock

Before 2011 there was no meaningful synchronization between
read/readdir/write/seek. Only in commit
ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
synchronization was added for SEEK_CUR by taking f_lock around
vfs_setpos().

Then in 2014 full synchronization between read/readdir/write/seek was
added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
for directories. At that point taking f_lock became unnecessary for such
files.

So only acquire f_lock for SEEK_CUR if this isn't a file that would have
acquired f_pos_lock if necessary.

Link: https://lore.kernel.org/r/20250207-daten-mahlzeit-99d2079864fb@brauner
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/file.c       | 10 ++++++++++
 fs/internal.h   |  1 +
 fs/read_write.c | 13 +++++++++----
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index d868cdb95d1e..44efdc8c1e27 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1182,6 +1182,16 @@ static inline bool file_needs_f_pos_lock(struct file *file)
 		(file_count(file) > 1 || file->f_op->iterate_shared);
 }
 
+bool file_seek_cur_needs_f_lock(struct file *file)
+{
+	if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
+		return false;
+
+	VFS_WARN_ON_ONCE((file_count(file) > 1) &&
+			 !mutex_is_locked(&file->f_pos_lock));
+	return true;
+}
+
 struct fd fdget_pos(unsigned int fd)
 {
 	struct fd f = fdget(fd);
diff --git a/fs/internal.h b/fs/internal.h
index 84607e7b05dc..1cb85a62c07f 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -338,3 +338,4 @@ static inline bool path_mounted(const struct path *path)
 	return path->mnt->mnt_root == path->dentry;
 }
 void file_f_owner_release(struct file *file);
+bool file_seek_cur_needs_f_lock(struct file *file);
diff --git a/fs/read_write.c b/fs/read_write.c
index a6133241dfb8..bb0ed26a0b3a 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -169,11 +169,16 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence,
 
 	if (whence == SEEK_CUR) {
 		/*
-		 * f_lock protects against read/modify/write race with
-		 * other SEEK_CURs. Note that parallel writes and reads
-		 * behave like SEEK_SET.
+		 * If the file requires locking via f_pos_lock we know
+		 * that mutual exclusion for SEEK_CUR on the same file
+		 * is guaranteed. If the file isn't locked, we take
+		 * f_lock to protect against f_pos races with other
+		 * SEEK_CURs.
 		 */
-		guard(spinlock)(&file->f_lock);
+		if (file_seek_cur_needs_f_lock(file)) {
+			guard(spinlock)(&file->f_lock);
+			return vfs_setpos(file, file->f_pos + offset, maxsize);
+		}
 		return vfs_setpos(file, file->f_pos + offset, maxsize);
 	}
 
-- 
2.47.2

Re: [PATCH] fs: don't needlessly acquire f_lock

[Jan Kara]

On Thu 13-02-25 16:10:55, Christian Brauner wrote:
> On Mon, Feb 10, 2025 at 03:58:17PM +0100, Jan Kara wrote:
> > On Mon 10-02-25 13:01:38, Christian Brauner wrote:
> > > On Fri, Feb 07, 2025 at 05:42:17PM +0100, Mateusz Guzik wrote:
> > > > On Fri, Feb 7, 2025 at 4:50 PM Jan Kara <jack@suse.cz> wrote:
> > > > >
> > > > > On Fri 07-02-25 15:10:33, Christian Brauner wrote:
> > > > > > Before 2011 there was no meaningful synchronization between
> > > > > > read/readdir/write/seek. Only in commit
> > > > > > ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
> > > > > > synchronization was added for SEEK_CUR by taking f_lock around
> > > > > > vfs_setpos().
> > > > > >
> > > > > > Then in 2014 full synchronization between read/readdir/write/seek was
> > > > > > added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
> > > > > > by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
> > > > > > for directories. At that point taking f_lock became unnecessary for such
> > > > > > files.
> > > > > >
> > > > > > So only acquire f_lock for SEEK_CUR if this isn't a file that would have
> > > > > > acquired f_pos_lock if necessary.
> > > > > >
> > > > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > > >
> > > > > ...
> > > > >
> > > > > >       if (whence == SEEK_CUR) {
> > > > > > +             bool locked;
> > > > > > +
> > > > > >               /*
> > > > > > -              * f_lock protects against read/modify/write race with
> > > > > > -              * other SEEK_CURs. Note that parallel writes and reads
> > > > > > -              * behave like SEEK_SET.
> > > > > > +              * If the file requires locking via f_pos_lock we know
> > > > > > +              * that mutual exclusion for SEEK_CUR on the same file
> > > > > > +              * is guaranteed. If the file isn't locked, we take
> > > > > > +              * f_lock to protect against f_pos races with other
> > > > > > +              * SEEK_CURs.
> > > > > >                */
> > > > > > -             guard(spinlock)(&file->f_lock);
> > > > > > -             return vfs_setpos(file, file->f_pos + offset, maxsize);
> > > > > > +             locked = (file->f_mode & FMODE_ATOMIC_POS) ||
> > > > > > +                      file->f_op->iterate_shared;
> > > > >
> > > > > As far as I understand the rationale this should match to
> > > > > file_needs_f_pos_lock() (or it can possibly be weaker) but it isn't obvious
> > > > > to me that's the case. After thinking about possibilities, I could convince
> > > > > myself that what you suggest is indeed safe but the condition being in two
> > > > > completely independent places and leading to subtle bugs if it gets out of
> > > > > sync seems a bit fragile to me.
> > > > >
> > > > 
> > > > A debug-only assert that the lock is held when expected should sort it out?
> > > 
> > > Good idea. Let me reuse the newly added infra:
> > > VFS_WARN_ON_ONCE(locked && !mutex_is_locked(&file->f_pos_lock));
> > 
> > Fine, but won't this actually trigger? file_needs_f_pos_lock() is:
> > 
> >         return (file->f_mode & FMODE_ATOMIC_POS) &&
> >                 (file_count(file) > 1 || file->f_op->iterate_shared);
> > 
> > and here you have:
> > 
> >         locked = (file->f_mode & FMODE_ATOMIC_POS) ||
> >                   file->f_op->iterate_shared;
> > 
> > So if (file->f_mode & FMODE_ATOMIC_POS) and (file_count(file) == 1),
> > file_needs_f_pos_lock() returns false but locked is true?
> 
> Sorry, I got lost in other mails.
> Yes, you're right. I had changed the patch in my tree to fix that.
> I'll append it here. Sorry about that. Tell me if that looks sane ok to
> you now.

Looks good to me! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: don't needlessly acquire f_lock

[Luis Henriques]

Hi Christian,

On Fri, Feb 07 2025, Christian Brauner wrote:

> Before 2011 there was no meaningful synchronization between
> read/readdir/write/seek. Only in commit
> ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
> synchronization was added for SEEK_CUR by taking f_lock around
> vfs_setpos().
>
> Then in 2014 full synchronization between read/readdir/write/seek was
> added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
> by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
> for directories. At that point taking f_lock became unnecessary for such
> files.
>
> So only acquire f_lock for SEEK_CUR if this isn't a file that would have
> acquired f_pos_lock if necessary.

I'm seeing the splat below with current master.  It's unlikely to be
related with this patch, but with recent overlayfs changes.  I'm just
dropping it here before looking, as maybe it has already been reported.

Cheers,
-- 
Luís

[  133.133745] ------------[ cut here ]------------
[  133.133855] WARNING: CPU: 6 PID: 246 at fs/file.c:1201 file_seek_cur_needs_f_lock+0x4a/0x60
[  133.133940] Modules linked in: virtiofs fuse
[  133.134009] CPU: 6 UID: 1000 PID: 246 Comm: ld Not tainted 6.15.0+ #124 PREEMPT(full) 
[  133.134110] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[  133.134235] RIP: 0010:file_seek_cur_needs_f_lock+0x4a/0x60
[  133.134286] Code: 00 48 ba fe ff ff ff ff ff ff bf 48 83 e8 01 48 39 c2 73 06 b8 01 00 00 00 c3 48 81 c7 90 00 00 00 e8 da 0e db ff 84 c0 75 ea <0f> 0b b8 01 00 00 00 c3 31 c0 c3 66 66 2e 0f 1f 84 00 00 00 00 00
[  133.134471] RSP: 0018:ffffc90000e67ea0 EFLAGS: 00010246
[  133.134526] RAX: 0000000000000000 RBX: fffffffffffffc01 RCX: 7fffffffffffffff
[  133.134683] RDX: bffffffffffffffe RSI: fffffffffffffc01 RDI: ffff888101bd1e90
[  133.135430] RBP: ffff888101bd1e00 R08: 00000000002a3988 R09: 0000000000000000
[  133.136172] R10: ffffc90000e67ed0 R11: 0000000000000000 R12: 7fffffffffffffff
[  133.136351] R13: ffff888101bd1e00 R14: ffff888105d823c0 R15: 0000000000000001
[  133.136433] FS:  00007fd7880d2b28(0000) GS:ffff8884ad411000(0000) knlGS:0000000000000000
[  133.136516] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  133.136586] CR2: 0000559b3af3a520 CR3: 0000000103cb1000 CR4: 0000000000750eb0
[  133.136667] PKRU: 55555554
[  133.136694] Call Trace:
[  133.136720]  <TASK>
[  133.136747]  generic_file_llseek_size+0x93/0x120
[  133.136802]  ovl_llseek+0x86/0xf0
[  133.136844]  ksys_lseek+0x39/0x90
[  133.136884]  do_syscall_64+0x73/0x2c0
[  133.136932]  entry_SYSCALL_64_after_hwframe+0x4b/0x53
[  133.136994] RIP: 0033:0x7fd788098262
[  133.137034] Code: 48 63 d2 48 63 ff 4d 63 c0 b8 09 01 00 00 0f 05 48 89 c7 e8 8a 80 fd ff 48 83 c4 08 c3 48 63 ff 48 63 d2 b8 08 00 00 00 0f 05 <48> 89 c7 e9 70 80 fd ff 8d 47 27 53 89 fb 83 f8 4e 76 27 b8 ec ff
[  133.137223] RSP: 002b:00007fffffaf82c8 EFLAGS: 00000283 ORIG_RAX: 0000000000000008
[  133.137302] RAX: ffffffffffffffda RBX: 00007fd787ba1010 RCX: 00007fd788098262
[  133.137385] RDX: 0000000000000001 RSI: fffffffffffffc01 RDI: 000000000000000f
[  133.137465] RBP: 0000000000000000 R08: 0000000000000064 R09: 00007fd787c3c6a0
[  133.137545] R10: 000000000000000e R11: 0000000000000283 R12: 00007fffffafa694
[  133.137625] R13: 0000000000000039 R14: 0000000000000038 R15: 00007fffffafaa79
[  133.137708]  </TASK>
[  133.137736] irq event stamp: 1034649
[  133.137776] hardirqs last  enabled at (1034657): [<ffffffff8133c642>] __up_console_sem+0x52/0x60
[  133.137872] hardirqs last disabled at (1034664): [<ffffffff8133c627>] __up_console_sem+0x37/0x60
[  133.137966] softirqs last  enabled at (1012640): [<ffffffff812c4884>] irq_exit_rcu+0x74/0x110
[  133.138064] softirqs last disabled at (1012633): [<ffffffff812c4884>] irq_exit_rcu+0x74/0x110
[  133.138161] ---[ end trace 0000000000000000 ]---

Re: [PATCH] fs: don't needlessly acquire f_lock

[Luis Henriques]

On Mon, Jun 02 2025, Luis Henriques wrote:

> Hi Christian,
>
> On Fri, Feb 07 2025, Christian Brauner wrote:
>
>> Before 2011 there was no meaningful synchronization between
>> read/readdir/write/seek. Only in commit
>> ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
>> synchronization was added for SEEK_CUR by taking f_lock around
>> vfs_setpos().
>>
>> Then in 2014 full synchronization between read/readdir/write/seek was
>> added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
>> by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
>> for directories. At that point taking f_lock became unnecessary for such
>> files.
>>
>> So only acquire f_lock for SEEK_CUR if this isn't a file that would have
>> acquired f_pos_lock if necessary.
>
> I'm seeing the splat below with current master.  It's unlikely to be
> related with this patch, but with recent overlayfs changes.  I'm just
> dropping it here before looking, as maybe it has already been reported.

OK, just to confirm that it looks like this is indeed due to this patch.
I can reproduce it easily, and I'm not sure why I haven't seen it before.

Cheers,
-- 
Luís


>
> Cheers,
> -- 
> Luís
>
> [  133.133745] ------------[ cut here ]------------
> [  133.133855] WARNING: CPU: 6 PID: 246 at fs/file.c:1201 file_seek_cur_needs_f_lock+0x4a/0x60
> [  133.133940] Modules linked in: virtiofs fuse
> [  133.134009] CPU: 6 UID: 1000 PID: 246 Comm: ld Not tainted 6.15.0+ #124 PREEMPT(full) 
> [  133.134110] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
> [  133.134235] RIP: 0010:file_seek_cur_needs_f_lock+0x4a/0x60
> [  133.134286] Code: 00 48 ba fe ff ff ff ff ff ff bf 48 83 e8 01 48 39 c2 73 06 b8 01 00 00 00 c3 48 81 c7 90 00 00 00 e8 da 0e db ff 84 c0 75 ea <0f> 0b b8 01 00 00 00 c3 31 c0 c3 66 66 2e 0f 1f 84 00 00 00 00 00
> [  133.134471] RSP: 0018:ffffc90000e67ea0 EFLAGS: 00010246
> [  133.134526] RAX: 0000000000000000 RBX: fffffffffffffc01 RCX: 7fffffffffffffff
> [  133.134683] RDX: bffffffffffffffe RSI: fffffffffffffc01 RDI: ffff888101bd1e90
> [  133.135430] RBP: ffff888101bd1e00 R08: 00000000002a3988 R09: 0000000000000000
> [  133.136172] R10: ffffc90000e67ed0 R11: 0000000000000000 R12: 7fffffffffffffff
> [  133.136351] R13: ffff888101bd1e00 R14: ffff888105d823c0 R15: 0000000000000001
> [  133.136433] FS:  00007fd7880d2b28(0000) GS:ffff8884ad411000(0000) knlGS:0000000000000000
> [  133.136516] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  133.136586] CR2: 0000559b3af3a520 CR3: 0000000103cb1000 CR4: 0000000000750eb0
> [  133.136667] PKRU: 55555554
> [  133.136694] Call Trace:
> [  133.136720]  <TASK>
> [  133.136747]  generic_file_llseek_size+0x93/0x120
> [  133.136802]  ovl_llseek+0x86/0xf0
> [  133.136844]  ksys_lseek+0x39/0x90
> [  133.136884]  do_syscall_64+0x73/0x2c0
> [  133.136932]  entry_SYSCALL_64_after_hwframe+0x4b/0x53
> [  133.136994] RIP: 0033:0x7fd788098262
> [  133.137034] Code: 48 63 d2 48 63 ff 4d 63 c0 b8 09 01 00 00 0f 05 48 89 c7 e8 8a 80 fd ff 48 83 c4 08 c3 48 63 ff 48 63 d2 b8 08 00 00 00 0f 05 <48> 89 c7 e9 70 80 fd ff 8d 47 27 53 89 fb 83 f8 4e 76 27 b8 ec ff
> [  133.137223] RSP: 002b:00007fffffaf82c8 EFLAGS: 00000283 ORIG_RAX: 0000000000000008
> [  133.137302] RAX: ffffffffffffffda RBX: 00007fd787ba1010 RCX: 00007fd788098262
> [  133.137385] RDX: 0000000000000001 RSI: fffffffffffffc01 RDI: 000000000000000f
> [  133.137465] RBP: 0000000000000000 R08: 0000000000000064 R09: 00007fd787c3c6a0
> [  133.137545] R10: 000000000000000e R11: 0000000000000283 R12: 00007fffffafa694
> [  133.137625] R13: 0000000000000039 R14: 0000000000000038 R15: 00007fffffafaa79
> [  133.137708]  </TASK>
> [  133.137736] irq event stamp: 1034649
> [  133.137776] hardirqs last  enabled at (1034657): [<ffffffff8133c642>] __up_console_sem+0x52/0x60
> [  133.137872] hardirqs last disabled at (1034664): [<ffffffff8133c627>] __up_console_sem+0x37/0x60
> [  133.137966] softirqs last  enabled at (1012640): [<ffffffff812c4884>] irq_exit_rcu+0x74/0x110
> [  133.138064] softirqs last disabled at (1012633): [<ffffffff812c4884>] irq_exit_rcu+0x74/0x110
> [  133.138161] ---[ end trace 0000000000000000 ]---

Re: [PATCH] fs: don't needlessly acquire f_lock

[Jan Kara]

On Mon 02-06-25 16:52:22, Luis Henriques wrote:
> On Mon, Jun 02 2025, Luis Henriques wrote:
> > Hi Christian,
> >
> > On Fri, Feb 07 2025, Christian Brauner wrote:
> >
> >> Before 2011 there was no meaningful synchronization between
> >> read/readdir/write/seek. Only in commit
> >> ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
> >> synchronization was added for SEEK_CUR by taking f_lock around
> >> vfs_setpos().
> >>
> >> Then in 2014 full synchronization between read/readdir/write/seek was
> >> added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
> >> by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
> >> for directories. At that point taking f_lock became unnecessary for such
> >> files.
> >>
> >> So only acquire f_lock for SEEK_CUR if this isn't a file that would have
> >> acquired f_pos_lock if necessary.
> >
> > I'm seeing the splat below with current master.  It's unlikely to be
> > related with this patch, but with recent overlayfs changes.  I'm just
> > dropping it here before looking, as maybe it has already been reported.
> 
> OK, just to confirm that it looks like this is indeed due to this patch.
> I can reproduce it easily, and I'm not sure why I haven't seen it before.

Thanks for report! Curious. This is:

        VFS_WARN_ON_ONCE((file_count(file) > 1) &&
                         !mutex_is_locked(&file->f_pos_lock));

Based on the fact this is ld(1) I expect this is a regular file.
Christian, cannot it happen that we race with dup2() so file_count is
increased after we've checked it in fdget_pos() and before we get to this
assert?

								Honza
> > [  133.133745] ------------[ cut here ]------------
> > [  133.133855] WARNING: CPU: 6 PID: 246 at fs/file.c:1201 file_seek_cur_needs_f_lock+0x4a/0x60
> > [  133.133940] Modules linked in: virtiofs fuse
> > [  133.134009] CPU: 6 UID: 1000 PID: 246 Comm: ld Not tainted 6.15.0+ #124 PREEMPT(full) 
> > [  133.134110] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
> > [  133.134235] RIP: 0010:file_seek_cur_needs_f_lock+0x4a/0x60
> > [  133.134286] Code: 00 48 ba fe ff ff ff ff ff ff bf 48 83 e8 01 48 39 c2 73 06 b8 01 00 00 00 c3 48 81 c7 90 00 00 00 e8 da 0e db ff 84 c0 75 ea <0f> 0b b8 01 00 00 00 c3 31 c0 c3 66 66 2e 0f 1f 84 00 00 00 00 00
> > [  133.134471] RSP: 0018:ffffc90000e67ea0 EFLAGS: 00010246
> > [  133.134526] RAX: 0000000000000000 RBX: fffffffffffffc01 RCX: 7fffffffffffffff
> > [  133.134683] RDX: bffffffffffffffe RSI: fffffffffffffc01 RDI: ffff888101bd1e90
> > [  133.135430] RBP: ffff888101bd1e00 R08: 00000000002a3988 R09: 0000000000000000
> > [  133.136172] R10: ffffc90000e67ed0 R11: 0000000000000000 R12: 7fffffffffffffff
> > [  133.136351] R13: ffff888101bd1e00 R14: ffff888105d823c0 R15: 0000000000000001
> > [  133.136433] FS:  00007fd7880d2b28(0000) GS:ffff8884ad411000(0000) knlGS:0000000000000000
> > [  133.136516] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [  133.136586] CR2: 0000559b3af3a520 CR3: 0000000103cb1000 CR4: 0000000000750eb0
> > [  133.136667] PKRU: 55555554
> > [  133.136694] Call Trace:
> > [  133.136720]  <TASK>
> > [  133.136747]  generic_file_llseek_size+0x93/0x120
> > [  133.136802]  ovl_llseek+0x86/0xf0
> > [  133.136844]  ksys_lseek+0x39/0x90
> > [  133.136884]  do_syscall_64+0x73/0x2c0
> > [  133.136932]  entry_SYSCALL_64_after_hwframe+0x4b/0x53
> > [  133.136994] RIP: 0033:0x7fd788098262
> > [  133.137034] Code: 48 63 d2 48 63 ff 4d 63 c0 b8 09 01 00 00 0f 05 48 89 c7 e8 8a 80 fd ff 48 83 c4 08 c3 48 63 ff 48 63 d2 b8 08 00 00 00 0f 05 <48> 89 c7 e9 70 80 fd ff 8d 47 27 53 89 fb 83 f8 4e 76 27 b8 ec ff
> > [  133.137223] RSP: 002b:00007fffffaf82c8 EFLAGS: 00000283 ORIG_RAX: 0000000000000008
> > [  133.137302] RAX: ffffffffffffffda RBX: 00007fd787ba1010 RCX: 00007fd788098262
> > [  133.137385] RDX: 0000000000000001 RSI: fffffffffffffc01 RDI: 000000000000000f
> > [  133.137465] RBP: 0000000000000000 R08: 0000000000000064 R09: 00007fd787c3c6a0
> > [  133.137545] R10: 000000000000000e R11: 0000000000000283 R12: 00007fffffafa694
> > [  133.137625] R13: 0000000000000039 R14: 0000000000000038 R15: 00007fffffafaa79
> > [  133.137708]  </TASK>
> > [  133.137736] irq event stamp: 1034649
> > [  133.137776] hardirqs last  enabled at (1034657): [<ffffffff8133c642>] __up_console_sem+0x52/0x60
> > [  133.137872] hardirqs last disabled at (1034664): [<ffffffff8133c627>] __up_console_sem+0x37/0x60
> > [  133.137966] softirqs last  enabled at (1012640): [<ffffffff812c4884>] irq_exit_rcu+0x74/0x110
> > [  133.138064] softirqs last disabled at (1012633): [<ffffffff812c4884>] irq_exit_rcu+0x74/0x110
> > [  133.138161] ---[ end trace 0000000000000000 ]---
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: don't needlessly acquire f_lock

[Christian Brauner]

On Tue, Jun 03, 2025 at 11:34:53AM +0200, Jan Kara wrote:
> On Mon 02-06-25 16:52:22, Luis Henriques wrote:
> > On Mon, Jun 02 2025, Luis Henriques wrote:
> > > Hi Christian,
> > >
> > > On Fri, Feb 07 2025, Christian Brauner wrote:
> > >
> > >> Before 2011 there was no meaningful synchronization between
> > >> read/readdir/write/seek. Only in commit
> > >> ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
> > >> synchronization was added for SEEK_CUR by taking f_lock around
> > >> vfs_setpos().
> > >>
> > >> Then in 2014 full synchronization between read/readdir/write/seek was
> > >> added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
> > >> by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
> > >> for directories. At that point taking f_lock became unnecessary for such
> > >> files.
> > >>
> > >> So only acquire f_lock for SEEK_CUR if this isn't a file that would have
> > >> acquired f_pos_lock if necessary.
> > >
> > > I'm seeing the splat below with current master.  It's unlikely to be
> > > related with this patch, but with recent overlayfs changes.  I'm just
> > > dropping it here before looking, as maybe it has already been reported.
> > 
> > OK, just to confirm that it looks like this is indeed due to this patch.
> > I can reproduce it easily, and I'm not sure why I haven't seen it before.
> 
> Thanks for report! Curious. This is:
> 
>         VFS_WARN_ON_ONCE((file_count(file) > 1) &&
>                          !mutex_is_locked(&file->f_pos_lock));
> 
> Based on the fact this is ld(1) I expect this is a regular file.
> Christian, cannot it happen that we race with dup2() so file_count is
> increased after we've checked it in fdget_pos() and before we get to this
> assert?

Yes I somehow thought the two of us had already discussed this and
either concluded to change it or drop the assert. Maybe I forgot that?
I'll remove the assert.

> 
> 								Honza
> > > [  133.133745] ------------[ cut here ]------------
> > > [  133.133855] WARNING: CPU: 6 PID: 246 at fs/file.c:1201 file_seek_cur_needs_f_lock+0x4a/0x60
> > > [  133.133940] Modules linked in: virtiofs fuse
> > > [  133.134009] CPU: 6 UID: 1000 PID: 246 Comm: ld Not tainted 6.15.0+ #124 PREEMPT(full) 
> > > [  133.134110] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
> > > [  133.134235] RIP: 0010:file_seek_cur_needs_f_lock+0x4a/0x60
> > > [  133.134286] Code: 00 48 ba fe ff ff ff ff ff ff bf 48 83 e8 01 48 39 c2 73 06 b8 01 00 00 00 c3 48 81 c7 90 00 00 00 e8 da 0e db ff 84 c0 75 ea <0f> 0b b8 01 00 00 00 c3 31 c0 c3 66 66 2e 0f 1f 84 00 00 00 00 00
> > > [  133.134471] RSP: 0018:ffffc90000e67ea0 EFLAGS: 00010246
> > > [  133.134526] RAX: 0000000000000000 RBX: fffffffffffffc01 RCX: 7fffffffffffffff
> > > [  133.134683] RDX: bffffffffffffffe RSI: fffffffffffffc01 RDI: ffff888101bd1e90
> > > [  133.135430] RBP: ffff888101bd1e00 R08: 00000000002a3988 R09: 0000000000000000
> > > [  133.136172] R10: ffffc90000e67ed0 R11: 0000000000000000 R12: 7fffffffffffffff
> > > [  133.136351] R13: ffff888101bd1e00 R14: ffff888105d823c0 R15: 0000000000000001
> > > [  133.136433] FS:  00007fd7880d2b28(0000) GS:ffff8884ad411000(0000) knlGS:0000000000000000
> > > [  133.136516] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > [  133.136586] CR2: 0000559b3af3a520 CR3: 0000000103cb1000 CR4: 0000000000750eb0
> > > [  133.136667] PKRU: 55555554
> > > [  133.136694] Call Trace:
> > > [  133.136720]  <TASK>
> > > [  133.136747]  generic_file_llseek_size+0x93/0x120
> > > [  133.136802]  ovl_llseek+0x86/0xf0
> > > [  133.136844]  ksys_lseek+0x39/0x90
> > > [  133.136884]  do_syscall_64+0x73/0x2c0
> > > [  133.136932]  entry_SYSCALL_64_after_hwframe+0x4b/0x53
> > > [  133.136994] RIP: 0033:0x7fd788098262
> > > [  133.137034] Code: 48 63 d2 48 63 ff 4d 63 c0 b8 09 01 00 00 0f 05 48 89 c7 e8 8a 80 fd ff 48 83 c4 08 c3 48 63 ff 48 63 d2 b8 08 00 00 00 0f 05 <48> 89 c7 e9 70 80 fd ff 8d 47 27 53 89 fb 83 f8 4e 76 27 b8 ec ff
> > > [  133.137223] RSP: 002b:00007fffffaf82c8 EFLAGS: 00000283 ORIG_RAX: 0000000000000008
> > > [  133.137302] RAX: ffffffffffffffda RBX: 00007fd787ba1010 RCX: 00007fd788098262
> > > [  133.137385] RDX: 0000000000000001 RSI: fffffffffffffc01 RDI: 000000000000000f
> > > [  133.137465] RBP: 0000000000000000 R08: 0000000000000064 R09: 00007fd787c3c6a0
> > > [  133.137545] R10: 000000000000000e R11: 0000000000000283 R12: 00007fffffafa694
> > > [  133.137625] R13: 0000000000000039 R14: 0000000000000038 R15: 00007fffffafaa79
> > > [  133.137708]  </TASK>
> > > [  133.137736] irq event stamp: 1034649
> > > [  133.137776] hardirqs last  enabled at (1034657): [<ffffffff8133c642>] __up_console_sem+0x52/0x60
> > > [  133.137872] hardirqs last disabled at (1034664): [<ffffffff8133c627>] __up_console_sem+0x37/0x60
> > > [  133.137966] softirqs last  enabled at (1012640): [<ffffffff812c4884>] irq_exit_rcu+0x74/0x110
> > > [  133.138064] softirqs last disabled at (1012633): [<ffffffff812c4884>] irq_exit_rcu+0x74/0x110
> > > [  133.138161] ---[ end trace 0000000000000000 ]---
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: [PATCH] fs: don't needlessly acquire f_lock

[Jan Kara]

On Wed 04-06-25 10:33:13, Christian Brauner wrote:
> On Tue, Jun 03, 2025 at 11:34:53AM +0200, Jan Kara wrote:
> > On Mon 02-06-25 16:52:22, Luis Henriques wrote:
> > > On Mon, Jun 02 2025, Luis Henriques wrote:
> > > > Hi Christian,
> > > >
> > > > On Fri, Feb 07 2025, Christian Brauner wrote:
> > > >
> > > >> Before 2011 there was no meaningful synchronization between
> > > >> read/readdir/write/seek. Only in commit
> > > >> ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
> > > >> synchronization was added for SEEK_CUR by taking f_lock around
> > > >> vfs_setpos().
> > > >>
> > > >> Then in 2014 full synchronization between read/readdir/write/seek was
> > > >> added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
> > > >> by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
> > > >> for directories. At that point taking f_lock became unnecessary for such
> > > >> files.
> > > >>
> > > >> So only acquire f_lock for SEEK_CUR if this isn't a file that would have
> > > >> acquired f_pos_lock if necessary.
> > > >
> > > > I'm seeing the splat below with current master.  It's unlikely to be
> > > > related with this patch, but with recent overlayfs changes.  I'm just
> > > > dropping it here before looking, as maybe it has already been reported.
> > > 
> > > OK, just to confirm that it looks like this is indeed due to this patch.
> > > I can reproduce it easily, and I'm not sure why I haven't seen it before.
> > 
> > Thanks for report! Curious. This is:
> > 
> >         VFS_WARN_ON_ONCE((file_count(file) > 1) &&
> >                          !mutex_is_locked(&file->f_pos_lock));
> > 
> > Based on the fact this is ld(1) I expect this is a regular file.
> > Christian, cannot it happen that we race with dup2() so file_count is
> > increased after we've checked it in fdget_pos() and before we get to this
> > assert?
> 
> Yes I somehow thought the two of us had already discussed this and
> either concluded to change it or drop the assert. Maybe I forgot that?
> I'll remove the assert.

I don't remember discussing this particular assert, I think it was a
different one of a similar kind :). Nevertheless I agree removing the
assert here is the right thing to do, it doesn't make too much sense in
this context.
								Honza

> 
> > 
> > 								Honza
> > > > [  133.133745] ------------[ cut here ]------------
> > > > [  133.133855] WARNING: CPU: 6 PID: 246 at fs/file.c:1201 file_seek_cur_needs_f_lock+0x4a/0x60
> > > > [  133.133940] Modules linked in: virtiofs fuse
> > > > [  133.134009] CPU: 6 UID: 1000 PID: 246 Comm: ld Not tainted 6.15.0+ #124 PREEMPT(full) 
> > > > [  133.134110] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
> > > > [  133.134235] RIP: 0010:file_seek_cur_needs_f_lock+0x4a/0x60
> > > > [  133.134286] Code: 00 48 ba fe ff ff ff ff ff ff bf 48 83 e8 01 48 39 c2 73 06 b8 01 00 00 00 c3 48 81 c7 90 00 00 00 e8 da 0e db ff 84 c0 75 ea <0f> 0b b8 01 00 00 00 c3 31 c0 c3 66 66 2e 0f 1f 84 00 00 00 00 00
> > > > [  133.134471] RSP: 0018:ffffc90000e67ea0 EFLAGS: 00010246
> > > > [  133.134526] RAX: 0000000000000000 RBX: fffffffffffffc01 RCX: 7fffffffffffffff
> > > > [  133.134683] RDX: bffffffffffffffe RSI: fffffffffffffc01 RDI: ffff888101bd1e90
> > > > [  133.135430] RBP: ffff888101bd1e00 R08: 00000000002a3988 R09: 0000000000000000
> > > > [  133.136172] R10: ffffc90000e67ed0 R11: 0000000000000000 R12: 7fffffffffffffff
> > > > [  133.136351] R13: ffff888101bd1e00 R14: ffff888105d823c0 R15: 0000000000000001
> > > > [  133.136433] FS:  00007fd7880d2b28(0000) GS:ffff8884ad411000(0000) knlGS:0000000000000000
> > > > [  133.136516] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > [  133.136586] CR2: 0000559b3af3a520 CR3: 0000000103cb1000 CR4: 0000000000750eb0
> > > > [  133.136667] PKRU: 55555554
> > > > [  133.136694] Call Trace:
> > > > [  133.136720]  <TASK>
> > > > [  133.136747]  generic_file_llseek_size+0x93/0x120
> > > > [  133.136802]  ovl_llseek+0x86/0xf0
> > > > [  133.136844]  ksys_lseek+0x39/0x90
> > > > [  133.136884]  do_syscall_64+0x73/0x2c0
> > > > [  133.136932]  entry_SYSCALL_64_after_hwframe+0x4b/0x53
> > > > [  133.136994] RIP: 0033:0x7fd788098262
> > > > [  133.137034] Code: 48 63 d2 48 63 ff 4d 63 c0 b8 09 01 00 00 0f 05 48 89 c7 e8 8a 80 fd ff 48 83 c4 08 c3 48 63 ff 48 63 d2 b8 08 00 00 00 0f 05 <48> 89 c7 e9 70 80 fd ff 8d 47 27 53 89 fb 83 f8 4e 76 27 b8 ec ff
> > > > [  133.137223] RSP: 002b:00007fffffaf82c8 EFLAGS: 00000283 ORIG_RAX: 0000000000000008
> > > > [  133.137302] RAX: ffffffffffffffda RBX: 00007fd787ba1010 RCX: 00007fd788098262
> > > > [  133.137385] RDX: 0000000000000001 RSI: fffffffffffffc01 RDI: 000000000000000f
> > > > [  133.137465] RBP: 0000000000000000 R08: 0000000000000064 R09: 00007fd787c3c6a0
> > > > [  133.137545] R10: 000000000000000e R11: 0000000000000283 R12: 00007fffffafa694
> > > > [  133.137625] R13: 0000000000000039 R14: 0000000000000038 R15: 00007fffffafaa79
> > > > [  133.137708]  </TASK>
> > > > [  133.137736] irq event stamp: 1034649
> > > > [  133.137776] hardirqs last  enabled at (1034657): [<ffffffff8133c642>] __up_console_sem+0x52/0x60
> > > > [  133.137872] hardirqs last disabled at (1034664): [<ffffffff8133c627>] __up_console_sem+0x37/0x60
> > > > [  133.137966] softirqs last  enabled at (1012640): [<ffffffff812c4884>] irq_exit_rcu+0x74/0x110
> > > > [  133.138064] softirqs last disabled at (1012633): [<ffffffff812c4884>] irq_exit_rcu+0x74/0x110
> > > > [  133.138161] ---[ end trace 0000000000000000 ]---
> > -- 
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: don't needlessly acquire f_lock

[Luis Henriques]

On Wed, Jun 04 2025, Jan Kara wrote:

> On Wed 04-06-25 10:33:13, Christian Brauner wrote:
>> On Tue, Jun 03, 2025 at 11:34:53AM +0200, Jan Kara wrote:
>> > On Mon 02-06-25 16:52:22, Luis Henriques wrote:
>> > > On Mon, Jun 02 2025, Luis Henriques wrote:
>> > > > Hi Christian,
>> > > >
>> > > > On Fri, Feb 07 2025, Christian Brauner wrote:
>> > > >
>> > > >> Before 2011 there was no meaningful synchronization between
>> > > >> read/readdir/write/seek. Only in commit
>> > > >> ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
>> > > >> synchronization was added for SEEK_CUR by taking f_lock around
>> > > >> vfs_setpos().
>> > > >>
>> > > >> Then in 2014 full synchronization between read/readdir/write/seek was
>> > > >> added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
>> > > >> by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
>> > > >> for directories. At that point taking f_lock became unnecessary for such
>> > > >> files.
>> > > >>
>> > > >> So only acquire f_lock for SEEK_CUR if this isn't a file that would have
>> > > >> acquired f_pos_lock if necessary.
>> > > >
>> > > > I'm seeing the splat below with current master.  It's unlikely to be
>> > > > related with this patch, but with recent overlayfs changes.  I'm just
>> > > > dropping it here before looking, as maybe it has already been reported.
>> > > 
>> > > OK, just to confirm that it looks like this is indeed due to this patch.
>> > > I can reproduce it easily, and I'm not sure why I haven't seen it before.
>> > 
>> > Thanks for report! Curious. This is:
>> > 
>> >         VFS_WARN_ON_ONCE((file_count(file) > 1) &&
>> >                          !mutex_is_locked(&file->f_pos_lock));
>> > 
>> > Based on the fact this is ld(1) I expect this is a regular file.
>> > Christian, cannot it happen that we race with dup2() so file_count is
>> > increased after we've checked it in fdget_pos() and before we get to this
>> > assert?
>> 
>> Yes I somehow thought the two of us had already discussed this and
>> either concluded to change it or drop the assert. Maybe I forgot that?
>> I'll remove the assert.
>
> I don't remember discussing this particular assert, I think it was a
> different one of a similar kind :). Nevertheless I agree removing the
> assert here is the right thing to do, it doesn't make too much sense in
> this context.

Awesome, thanks for looking into this Jan and Christian. 

Cheers,
-- 
Luís

[PATCH] fs: drop assert in file_seek_cur_needs_f_lock

[Luis Henriques]

The assert in function file_seek_cur_needs_f_lock() can be triggered very
easily because, as Jan Kara suggested, the file reference may get
incremented after checking it with fdget_pos().

Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
Signed-off-by: Luis Henriques <luis@igalia.com>
---
Hi Christian,

It wasn't clear whether you'd be queueing this fix yourself.  Since I don't
see it on vfs.git, I decided to explicitly send the patch so that it doesn't
slip through the cracks.

Cheers,
-- 
Luis

 fs/file.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 3a3146664cf3..075f07bdc977 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1198,8 +1198,6 @@ bool file_seek_cur_needs_f_lock(struct file *file)
 	if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
 		return false;
 
-	VFS_WARN_ON_ONCE((file_count(file) > 1) &&
-			 !mutex_is_locked(&file->f_pos_lock));
 	return true;
 }
 

Re: [PATCH] fs: drop assert in file_seek_cur_needs_f_lock

[Jan Kara]

On Thu 12-06-25 10:41:01, Luis Henriques wrote:
> The assert in function file_seek_cur_needs_f_lock() can be triggered very
> easily because, as Jan Kara suggested, the file reference may get
> incremented after checking it with fdget_pos().
> 
> Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
> Signed-off-by: Luis Henriques <luis@igalia.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> ---
> Hi Christian,
> 
> It wasn't clear whether you'd be queueing this fix yourself.  Since I don't
> see it on vfs.git, I decided to explicitly send the patch so that it doesn't
> slip through the cracks.
> 
> Cheers,
> -- 
> Luis
> 
>  fs/file.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 3a3146664cf3..075f07bdc977 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -1198,8 +1198,6 @@ bool file_seek_cur_needs_f_lock(struct file *file)
>  	if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
>  		return false;
>  
> -	VFS_WARN_ON_ONCE((file_count(file) > 1) &&
> -			 !mutex_is_locked(&file->f_pos_lock));
>  	return true;
>  }
>  
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: drop assert in file_seek_cur_needs_f_lock

[Christian Brauner]

On Thu, 12 Jun 2025 10:41:01 +0100, Luis Henriques wrote:
> The assert in function file_seek_cur_needs_f_lock() can be triggered very
> easily because, as Jan Kara suggested, the file reference may get
> incremented after checking it with fdget_pos().
> 
> 

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] fs: drop assert in file_seek_cur_needs_f_lock
      https://git.kernel.org/vfs/vfs/c/ec86bba684b1

Re: [PATCH] fs: drop assert in file_seek_cur_needs_f_lock

[Mateusz Guzik]

On Thu, Jun 12, 2025 at 10:41:01AM +0100, Luis Henriques wrote:
> The assert in function file_seek_cur_needs_f_lock() can be triggered very
> easily because, as Jan Kara suggested, the file reference may get
> incremented after checking it with fdget_pos().
> 
> Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
> Signed-off-by: Luis Henriques <luis@igalia.com>
> ---
> Hi Christian,
> 
> It wasn't clear whether you'd be queueing this fix yourself.  Since I don't
> see it on vfs.git, I decided to explicitly send the patch so that it doesn't
> slip through the cracks.
> 
> Cheers,
> -- 
> Luis
> 
>  fs/file.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 3a3146664cf3..075f07bdc977 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -1198,8 +1198,6 @@ bool file_seek_cur_needs_f_lock(struct file *file)
>  	if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
>  		return false;
>  
> -	VFS_WARN_ON_ONCE((file_count(file) > 1) &&
> -			 !mutex_is_locked(&file->f_pos_lock));
>  	return true;
>  }
>  

There this justifies the change.

fdget_pos() can only legally skip locking if it determines to be in
position where nobody else can operate on the same file obj, meaning
file_count(file) == 1 and it can't go up. Otherwise the lock is taken.

Or to put it differently, fdget_pos() NOT taking the lock and new refs
showing up later is a bug.

I don't believe anything of the sort is happening here.

Instead, overlayfs is playing games and *NOT* going through fdget_pos():

	ovl_inode_lock(inode);
        realfile = ovl_real_file(file);
	[..]
        ret = vfs_llseek(realfile, offset, whence);

Given the custom inode locking around the call, it may be any other
locking is unnecessary and the code happens to be correct despite the
splat.

I think the safest way out with some future-proofing is to in fact *add*
the locking in ovl_llseek() to shut up the assert -- personally I find
it uneasy there is some underlying file obj flying around.

Even if ultimately the assert has to go, the proposed commit message
does not justify it.

Re: [PATCH] fs: drop assert in file_seek_cur_needs_f_lock

[Mateusz Guzik]

On Thu, Jun 12, 2025 at 3:55 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> On Thu, Jun 12, 2025 at 10:41:01AM +0100, Luis Henriques wrote:
> > The assert in function file_seek_cur_needs_f_lock() can be triggered very
> > easily because, as Jan Kara suggested, the file reference may get
> > incremented after checking it with fdget_pos().
> >
> > Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
> > Signed-off-by: Luis Henriques <luis@igalia.com>
> > ---
> > Hi Christian,
> >
> > It wasn't clear whether you'd be queueing this fix yourself.  Since I don't
> > see it on vfs.git, I decided to explicitly send the patch so that it doesn't
> > slip through the cracks.
> >
> > Cheers,
> > --
> > Luis
> >
> >  fs/file.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/fs/file.c b/fs/file.c
> > index 3a3146664cf3..075f07bdc977 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -1198,8 +1198,6 @@ bool file_seek_cur_needs_f_lock(struct file *file)
> >       if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
> >               return false;
> >
> > -     VFS_WARN_ON_ONCE((file_count(file) > 1) &&
> > -                      !mutex_is_locked(&file->f_pos_lock));
> >       return true;
> >  }
> >
>
> There this justifies the change.
>

Huh. scratch this sentence. I stand by the rest. :)

> fdget_pos() can only legally skip locking if it determines to be in
> position where nobody else can operate on the same file obj, meaning
> file_count(file) == 1 and it can't go up. Otherwise the lock is taken.
>
> Or to put it differently, fdget_pos() NOT taking the lock and new refs
> showing up later is a bug.
>
> I don't believe anything of the sort is happening here.
>
> Instead, overlayfs is playing games and *NOT* going through fdget_pos():
>
>         ovl_inode_lock(inode);
>         realfile = ovl_real_file(file);
>         [..]
>         ret = vfs_llseek(realfile, offset, whence);
>
> Given the custom inode locking around the call, it may be any other
> locking is unnecessary and the code happens to be correct despite the
> splat.
>
> I think the safest way out with some future-proofing is to in fact *add*
> the locking in ovl_llseek() to shut up the assert -- personally I find
> it uneasy there is some underlying file obj flying around.
>
> Even if ultimately the assert has to go, the proposed commit message
> does not justify it.



-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] fs: drop assert in file_seek_cur_needs_f_lock

[Jan Kara]

On Thu 12-06-25 15:55:40, Mateusz Guzik wrote:
> On Thu, Jun 12, 2025 at 10:41:01AM +0100, Luis Henriques wrote:
> > The assert in function file_seek_cur_needs_f_lock() can be triggered very
> > easily because, as Jan Kara suggested, the file reference may get
> > incremented after checking it with fdget_pos().
> > 
> > Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
> > Signed-off-by: Luis Henriques <luis@igalia.com>
> > ---
> > Hi Christian,
> > 
> > It wasn't clear whether you'd be queueing this fix yourself.  Since I don't
> > see it on vfs.git, I decided to explicitly send the patch so that it doesn't
> > slip through the cracks.
> > 
> > Cheers,
> > -- 
> > Luis
> > 
> >  fs/file.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/fs/file.c b/fs/file.c
> > index 3a3146664cf3..075f07bdc977 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -1198,8 +1198,6 @@ bool file_seek_cur_needs_f_lock(struct file *file)
> >  	if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
> >  		return false;
> >  
> > -	VFS_WARN_ON_ONCE((file_count(file) > 1) &&
> > -			 !mutex_is_locked(&file->f_pos_lock));
> >  	return true;
> >  }
> 
> fdget_pos() can only legally skip locking if it determines to be in
> position where nobody else can operate on the same file obj, meaning
> file_count(file) == 1 and it can't go up. Otherwise the lock is taken.
> 
> Or to put it differently, fdget_pos() NOT taking the lock and new refs
> showing up later is a bug.

I mostly agree and as I've checked again, this indeed seems to be the case
as fdget() will increment f_ref if file table is shared with another thread
and thus file_needs_f_pos_lock() returns true whenever there are more
threads sharing the file table or if the struct file is dupped to another
fd. That being said I find the assertion in file_seek_cur_needs_f_lock()
misplaced - it just doesn't make sense in that place to me.
 
> I don't believe anything of the sort is happening here.
> 
> Instead, overlayfs is playing games and *NOT* going through fdget_pos():
> 
> 	ovl_inode_lock(inode);
>         realfile = ovl_real_file(file);
> 	[..]
>         ret = vfs_llseek(realfile, offset, whence);
> 
> Given the custom inode locking around the call, it may be any other
> locking is unnecessary and the code happens to be correct despite the
> splat.

Right and good spotting. That's indeed more likely explanation than mine.
Actually custom locking around llseek isn't all that uncommon (mostly for
historical reasons AFAIK but that's another story).

> I think the safest way out with some future-proofing is to in fact *add*
> the locking in ovl_llseek() to shut up the assert -- personally I find
> it uneasy there is some underlying file obj flying around.

Well, if you grep for vfs_llseek(), you'll see there are much more calls to
it in the kernel than overlayfs. These callers outside of fs/read_write.c
are responsible for their locking. So I don't think keeping the assert in
file_seek_cur_needs_f_lock() makes any sense. If anything I'd be open to
putting it in fdput_pos() or something like that.

> Even if ultimately the assert has to go, the proposed commit message
> does not justify it.

I guess the commit message could be improved. Something like:

The assert in function file_seek_cur_needs_f_lock() can be triggered very
easily because there are many users of vfs_llseek() (such as overlayfs)
that do their custom locking around llseek instead of relying on
fdget_pos(). Just drop the overzealous assertion.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: drop assert in file_seek_cur_needs_f_lock

[Luis Henriques]

On Thu, Jun 12 2025, Jan Kara wrote:

> On Thu 12-06-25 15:55:40, Mateusz Guzik wrote:
>> On Thu, Jun 12, 2025 at 10:41:01AM +0100, Luis Henriques wrote:
>> > The assert in function file_seek_cur_needs_f_lock() can be triggered very
>> > easily because, as Jan Kara suggested, the file reference may get
>> > incremented after checking it with fdget_pos().
>> > 
>> > Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
>> > Signed-off-by: Luis Henriques <luis@igalia.com>
>> > ---
>> > Hi Christian,
>> > 
>> > It wasn't clear whether you'd be queueing this fix yourself.  Since I don't
>> > see it on vfs.git, I decided to explicitly send the patch so that it doesn't
>> > slip through the cracks.
>> > 
>> > Cheers,
>> > -- 
>> > Luis
>> > 
>> >  fs/file.c | 2 --
>> >  1 file changed, 2 deletions(-)
>> > 
>> > diff --git a/fs/file.c b/fs/file.c
>> > index 3a3146664cf3..075f07bdc977 100644
>> > --- a/fs/file.c
>> > +++ b/fs/file.c
>> > @@ -1198,8 +1198,6 @@ bool file_seek_cur_needs_f_lock(struct file *file)
>> >  	if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
>> >  		return false;
>> >  
>> > -	VFS_WARN_ON_ONCE((file_count(file) > 1) &&
>> > -			 !mutex_is_locked(&file->f_pos_lock));
>> >  	return true;
>> >  }
>> 
>> fdget_pos() can only legally skip locking if it determines to be in
>> position where nobody else can operate on the same file obj, meaning
>> file_count(file) == 1 and it can't go up. Otherwise the lock is taken.
>> 
>> Or to put it differently, fdget_pos() NOT taking the lock and new refs
>> showing up later is a bug.
>
> I mostly agree and as I've checked again, this indeed seems to be the case
> as fdget() will increment f_ref if file table is shared with another thread
> and thus file_needs_f_pos_lock() returns true whenever there are more
> threads sharing the file table or if the struct file is dupped to another
> fd. That being said I find the assertion in file_seek_cur_needs_f_lock()
> misplaced - it just doesn't make sense in that place to me.
>  
>> I don't believe anything of the sort is happening here.
>> 
>> Instead, overlayfs is playing games and *NOT* going through fdget_pos():
>> 
>> 	ovl_inode_lock(inode);
>>         realfile = ovl_real_file(file);
>> 	[..]
>>         ret = vfs_llseek(realfile, offset, whence);
>> 
>> Given the custom inode locking around the call, it may be any other
>> locking is unnecessary and the code happens to be correct despite the
>> splat.
>
> Right and good spotting. That's indeed more likely explanation than mine.
> Actually custom locking around llseek isn't all that uncommon (mostly for
> historical reasons AFAIK but that's another story).
>
>> I think the safest way out with some future-proofing is to in fact *add*
>> the locking in ovl_llseek() to shut up the assert -- personally I find
>> it uneasy there is some underlying file obj flying around.
>
> Well, if you grep for vfs_llseek(), you'll see there are much more calls to
> it in the kernel than overlayfs. These callers outside of fs/read_write.c
> are responsible for their locking. So I don't think keeping the assert in
> file_seek_cur_needs_f_lock() makes any sense. If anything I'd be open to
> putting it in fdput_pos() or something like that.

Thank you Mateusz and Honza for looking into this.  Overlayfs was indeed
my initial suspect, but I had two reasons for thinking that the assert was
the problem: 1) that code was there for quite some time and 2) nobody else
was reporting this issue.

>> Even if ultimately the assert has to go, the proposed commit message
>> does not justify it.
>
> I guess the commit message could be improved. Something like:
>
> The assert in function file_seek_cur_needs_f_lock() can be triggered very
> easily because there are many users of vfs_llseek() (such as overlayfs)
> that do their custom locking around llseek instead of relying on
> fdget_pos(). Just drop the overzealous assertion.

Thanks, makes more sense.

Christian, do you prefer me to resend the patch or is it easier for you to
just amend the commit?  (Though, to be fair, the authorship could also be
changed as I mostly reported the issue and tested!)

Cheers,
-- 
Luís

Re: [PATCH] fs: drop assert in file_seek_cur_needs_f_lock

[Mateusz Guzik]

On Thu, Jun 12, 2025 at 8:07 PM Luis Henriques <luis@igalia.com> wrote:
> > I guess the commit message could be improved. Something like:
> >
> > The assert in function file_seek_cur_needs_f_lock() can be triggered very
> > easily because there are many users of vfs_llseek() (such as overlayfs)
> > that do their custom locking around llseek instead of relying on
> > fdget_pos(). Just drop the overzealous assertion.
>
> Thanks, makes more sense.
>
> Christian, do you prefer me to resend the patch or is it easier for you to
> just amend the commit?  (Though, to be fair, the authorship could also be
> changed as I mostly reported the issue and tested!)
>

How about leaving a trace in the code.

For example a comment of this sort in place of the assert:
Note that we are not guaranteed to be called after fdget_pos() on this
file obj, in which case the caller is expected to provide the
appropriate locking.

I find it fishy af that a rando fs is playing games with the file obj
*and* the fact that games are played is not assertable, but at least
people can be warned.
-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] fs: drop assert in file_seek_cur_needs_f_lock

[Jan Kara]

On Thu 12-06-25 23:35:09, Mateusz Guzik wrote:
> On Thu, Jun 12, 2025 at 8:07 PM Luis Henriques <luis@igalia.com> wrote:
> > > I guess the commit message could be improved. Something like:
> > >
> > > The assert in function file_seek_cur_needs_f_lock() can be triggered very
> > > easily because there are many users of vfs_llseek() (such as overlayfs)
> > > that do their custom locking around llseek instead of relying on
> > > fdget_pos(). Just drop the overzealous assertion.
> >
> > Thanks, makes more sense.
> >
> > Christian, do you prefer me to resend the patch or is it easier for you to
> > just amend the commit?  (Though, to be fair, the authorship could also be
> > changed as I mostly reported the issue and tested!)
> >
> 
> How about leaving a trace in the code.
> 
> For example a comment of this sort in place of the assert:
> Note that we are not guaranteed to be called after fdget_pos() on this
> file obj, in which case the caller is expected to provide the
> appropriate locking.

I'm not opposed to that.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2] fs: drop assert in file_seek_cur_needs_f_lock

[Luis Henriques]

The assert in function file_seek_cur_needs_f_lock() can be triggered very
easily because there are many users of vfs_llseek() (such as overlayfs)
that do their custom locking around llseek instead of relying on
fdget_pos(). Just drop the overzealous assertion.

Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
Suggested-by: Jan Kara <jack@suse.cz>
Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
Signed-off-by: Luis Henriques <luis@igalia.com>
---
Hi!

As suggested by Mateusz, I'm adding a comment (also suggested by him!) to
replace the assertion.  I'm also adding the 'Suggested-by:' tags, although
I'm not sure it's the correct tag to use -- the authorship of this patch
isn't really clear at this point :-)

 fs/file.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 3a3146664cf3..b6db031545e6 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1198,8 +1198,12 @@ bool file_seek_cur_needs_f_lock(struct file *file)
 	if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
 		return false;
 
-	VFS_WARN_ON_ONCE((file_count(file) > 1) &&
-			 !mutex_is_locked(&file->f_pos_lock));
+	/*
+	 * Note that we are not guaranteed to be called after fdget_pos() on
+	 * this file obj, in which case the caller is expected to provide the
+	 * appropriate locking.
+	 */
+
 	return true;
 }
 

Re: [PATCH v2] fs: drop assert in file_seek_cur_needs_f_lock

[Mateusz Guzik]

On Fri, Jun 13, 2025 at 12:11 PM Luis Henriques <luis@igalia.com> wrote:
>
> The assert in function file_seek_cur_needs_f_lock() can be triggered very
> easily because there are many users of vfs_llseek() (such as overlayfs)
> that do their custom locking around llseek instead of relying on
> fdget_pos(). Just drop the overzealous assertion.
>
> Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
> Suggested-by: Jan Kara <jack@suse.cz>
> Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
> Signed-off-by: Luis Henriques <luis@igalia.com>
> ---
> Hi!
>
> As suggested by Mateusz, I'm adding a comment (also suggested by him!) to
> replace the assertion.  I'm also adding the 'Suggested-by:' tags, although
> I'm not sure it's the correct tag to use -- the authorship of this patch
> isn't really clear at this point :-)
>
>  fs/file.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/file.c b/fs/file.c
> index 3a3146664cf3..b6db031545e6 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -1198,8 +1198,12 @@ bool file_seek_cur_needs_f_lock(struct file *file)
>         if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
>                 return false;
>
> -       VFS_WARN_ON_ONCE((file_count(file) > 1) &&
> -                        !mutex_is_locked(&file->f_pos_lock));
> +       /*
> +        * Note that we are not guaranteed to be called after fdget_pos() on
> +        * this file obj, in which case the caller is expected to provide the
> +        * appropriate locking.
> +        */
> +
>         return true;
>  }
>

well i think this is fine, obviously ;-)

I was hoping a native speaker would do some touch ups on the comment though.
-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH v2] fs: drop assert in file_seek_cur_needs_f_lock

[Christian Brauner]

On Fri, 13 Jun 2025 11:11:11 +0100, Luis Henriques wrote:
> The assert in function file_seek_cur_needs_f_lock() can be triggered very
> easily because there are many users of vfs_llseek() (such as overlayfs)
> that do their custom locking around llseek instead of relying on
> fdget_pos(). Just drop the overzealous assertion.
> 
> 

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] fs: drop assert in file_seek_cur_needs_f_lock
      https://git.kernel.org/vfs/vfs/c/dd2d6b7f6f51