Re: [PATCH 0/4] Transaction aborts in free-space-tree.c

[David Sterba <dsterba@suse.cz>]

On Fri, May 02, 2025 at 03:32:52PM +0200, Daniel Vacek wrote:
> On Fri, 2 May 2025 at 15:24, David Sterba <dsterba@suse.cz> wrote:
> >
> > On Fri, May 02, 2025 at 03:15:49PM +0200, Daniel Vacek wrote:
> > > On Wed, 30 Apr 2025 at 18:45, David Sterba <dsterba@suse.com> wrote:
> > > >
> > > > Fix the transaction abort pattern in free-space-tree, it's been there
> > > > from the beginning and not conforming to the pattern we use elsewhere.
> > > >
> > > > David Sterba (4):
> > > >   btrfs: move transaction aborts to the error site in
> > > >     convert_free_space_to_bitmaps()
> > > >   btrfs: move transaction aborts to the error site in
> > > >     convert_free_space_to_extents()
> > > >   btrfs: move transaction aborts to the error site in
> > > >     remove_from_free_space_tree()
> > > >   btrfs: move transaction aborts to the error site in
> > > >     add_to_free_space_tree()
> > > >
> > > >  fs/btrfs/free-space-tree.c | 48 +++++++++++++++++++++++++-------------
> > > >  1 file changed, 32 insertions(+), 16 deletions(-)
> > >
> > > This looks like unnecessary duplicating the code. Shall we rather go
> > > the other way around?
> >
> > What do you mean? There's some smilarity among the functions so yeah
> > the add/remove pairs can be merged to one, but this is orthogonal to the
> > transaction abot calls.
> 
> Not that. I meant the code looks simpler without these patches. If
> this is the pattern used elsewhere, maybe we should rather change
> elsewhere to follow free-space-tree.c?

https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#error-handling-and-transaction-abort

"Please keep all transaction abort exactly at the place where they happen
and do not merge them to one. This pattern should be used everywhere and
is important when debugging because we can pinpoint the line in the code
from the syslog message and do not have to guess which way it got to the
merged call."