Re: [PATCH v3 1/4] btrfs: make sure all ordered extents beyond EOF is properly truncated

[David Sterba <dsterba@suse.cz>]

On Sat, Nov 22, 2025 at 07:16:06AM +1030, Qu Wenruo wrote:
> 在 2025/11/22 06:55, Daniel Vacek 写道:
> > On Fri, 21 Nov 2025 at 20:28, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> >> 在 2025/11/22 02:08, David Sterba 写道:
> >>> On Fri, Nov 21, 2025 at 11:55:58AM +0000, Filipe Manana wrote:
> >>>>> +               /*
> >>>>> +                * We have just run delalloc before getting here, so there must
> >>>>> +                * be an ordered extent.
> >>>>> +                */
> >>>>> +               ASSERT(ordered != NULL);
> >>>>> +               scoped_guard(spinlock, &inode->ordered_tree_lock) {
> >>>>> +                       set_bit(BTRFS_ORDERED_TRUNCATED, &ordered->flags);
> >>>>> +                       ordered->truncated_len = min(ordered->truncated_len,
> >>>>> +                                                    cur - ordered->file_offset);
> >>>>> +               }
> >>>>
> >>>> I thought we had not made a decision yet to not use this new fancy locking yet.
> >>>> In this case where it's a very short critical section, it doesn't
> >>>> bring any advantage over using explicit spin_lock/unlock, and adds one
> >>>> extra level of indentation.
> >>>
> >>> Agreed, this looks like an anti-pattern of the scoped locking.
> >>>
> >>
> >> I think one is free to use whatever style as long as there is no mixed
> >> style in the same function.
> > 
> > I've got a hard objection here. If there is(?) any granularity using
> > guards vs. explicit locking then, IMO, it should be per given lock.
> > 
> > Ie, given `ordered_tree_lock` - either it should always use the RAII
> > style *OR* it should always use the explicit style. But it should
> > never mix one style in one function and the other style in another
> > function. That would only make it really messy looking for
> > interactions and race bugs / missing/misplaced locking - simply
> > general debugging.
> 
> Then check fs/*.c.
> 
> There are guard(rcu) and rcu_read_lock() usage mixed in different files.
> 
> E.g. in fs/namespace.c it's using guard(rcu) and scoped_guard(rcu), 
> meanwhile still having regular rcu_read_lock().
> 
> There are more counter-examples than you know.
> We're not the first subsystem to face the new RAII styles, and I hope we 
> will not be the last either.

We take inspiration from other subsystems but that some coding style is
done in another subsystem does not mean we have to do that too. If
fs/*.c people decide to use it everywhere then so be it.

I was hesitant to introduce the auto cleaning for btrfs_path or kfree
but so far I found it working. The locking guards are quite different to
that and I don't seem to get any liking in it

> [...]
> >>
> >> I'm not saying we should change to the new RAII style immediately with a
> >> huge patch nor everyone should accept it immediately, but to gradually
> >> use the new style in new codes, with the usual proper review/testing
> >> procedures to keep the correctness.
> > 
> > I would understand if you introduced a _new_ lock and started using it
> > with the RAII style - setting the example. But if you're grabbing a
> > lock which is always acquired using the explicit style, just stick to
> > it and keep the style consistent throughout all the callsites, the
> > whole code base. This makes it _easier_ to crosscheck, IMO.
> 
> Then check kernfs_root::kernfs_rwsem, it also has mixed RAII and old styles.
> 
> We should all remember there are always subsystems before us facing this 
> challenge, and if they are fine embrace the new style gradually, I see 
> no reason why we can not.
> 
> We're just a regular subsystem in the kernel, not some god-chosen 
> special one, we do not live in a bubble.

Honestly, this is a weird argument to make. There are local coding
styles that are their own bubbles, look at net/* with their own special
commenting style and mandatory reverse xmass tree sort of variables (I
think that one has been adopted by other subsystems too). Contributing to
those subsystems means following their style. If you want an example
of a filesystem with unique code formatting style then go no further
than XFS.

I think we've been doing fine in btrfs with the style consistency and
incremental updates of old code when possible. This means we have fewer
choices for personal "creative expression" how the code is written but
in the long run it makes the code look better.

The problem with the guards is that there's no one good way for their
general use, i.e. the problem of mixing with explicit lock/unlock,
trylock, unlock/lock order, etc. Allowing both styles will lead to
inconsistent style based on personal preferences again.

As said on slack, we'll get to a conclusion.