Re: [PATCH] btrfs: properly split extent_map for REQ_OP_ZONE_APPEND

[Naohiro Aota <Naohiro.Aota@wdc.com>]

On Wed, Jul 28, 2021 at 05:47:03PM +0200, David Sterba wrote:
> On Thu, Jul 01, 2021 at 05:55:51PM +0100, Filipe Manana wrote:
> > > > +       if (pre) {
> > > > +               /* Insert the middle extent_map */
> > > > +               split_mid->start = em->start + pre;
> > > > +               split_mid->len = em->len - pre - post;
> > > > +               split_mid->orig_start = split_mid->start;
> > > > +               split_mid->block_start = em->block_start + pre;
> > > > +               split_mid->block_len = split_mid->len;
> > > > +               split_mid->orig_block_len = split_mid->block_len;
> > > > +               split_mid->ram_bytes = split_mid->len;
> > > > +               split_mid->flags = flags;
> > > > +               split_mid->compress_type = em->compress_type;
> > > > +               split_mid->generation = em->generation;
> > > > +               add_extent_mapping(em_tree, split_mid, modified);
> > > > +       }
> > > > +
> > > > +       if (post) {
> > > > +               split_post->start = em->start + em->len - post;
> > > > +               split_post->len = post;
> > > > +               split_post->orig_start = split_post->start;
> > > > +               split_post->block_start = em->block_start + em->len - post;
> > > > +               split_post->block_len = split_post->len;
> > > > +               split_post->orig_block_len = split_post->block_len;
> > > > +               split_post->ram_bytes = split_post->len;
> > > > +               split_post->flags = flags;
> > > > +               split_post->compress_type = em->compress_type;
> > > > +               split_post->generation = em->generation;
> > > > +               add_extent_mapping(em_tree, split_post, modified);
> > > > +       }
> > >
> > > So this happens when running delalloc, after creating the original
> > > extent map and ordered extent, the original "em" must have had the
> > > PINNED flag set.
> > >
> > > The "pre" and "post" extent maps should have the PINNED flag set. It's
> > > important to have the flag set to prevent extent map merging, which
> > > could result in a log corruption if the file is being fsync'ed
> > > multiple times in the current transaction and running delalloc was
> > > triggered precisely by an fsync. The corruption result would be
> > > logging extent items with overlapping ranges, since we construct them
> > > based on extent maps, and that's why we have the PINNED flag to
> > > prevent merging.
> > 
> > Well, it actually happens that merging should not happen because the
> > original extent map was in the list of modified extents, and so should
> > be the new extent maps.
> > But we are really supposed to have the PINNED flag from the moment we
> > run delalloc and create a new extent map until the respective ordered
> > extent completes and unpins it.
> > 
> > Also EXTENT_FLAG_LOGGING should not be set at this point - if it were
> > we would screw up with a task logging the extent map.
> > 
> > Maybe assert that it is not set in the original extent map?
> > And also assert that the original em is in the list of modified
> > extents and has the PINNED flag set?
> 
> Agreed, the asserts should be here, Naohiro, please send a followup,
> thanks.

Sure. I'll soon send an update.