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

[Luis Henriques <luis@igalia.com>]

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