Re: [PATCH v2 07/11] btrfs: remove extent_map::block_start member

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/5/17 02:58, Filipe Manana 写道:
> On Fri, May 3, 2024 at 7:02 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> The member extent_map::block_start can be calculated from
>> extent_map::disk_bytenr + extent_map::offset for regular extents.
>> And otherwise just extent_map::disk_bytenr.
>>
>> And this is already validated by the validate_extent_map().
>> Now we can remove the member.
>>
>> However there is a special case in btrfs_alloc_ordered_extent(), where
>> we intentionally pass a pseudo ordered extent, exploiting the fact that
>> for NOCOW/PREALLOC writes we do not rely on ordered extent members to
>> update the file extent items.
>>
>> And that's the only way to pass btrfs_extract_ordered_extent(), as it
>> doesn't accept any ordered extent with an offset.
>>
>> For now we will keep the old pseudo ordered extent members, and leave
>> the cleanup of it for the future.
>
> These last 3 paragraphs, the rant about NOCOW writes and ordered
> extents, seem unnecessary to me.
>
> This is just how things work, and for me it makes total sense that an
> ordered extent for a NOCOW write does not represent the entirety of an
> existing file extent item.
> NOCOW writes don't create new file extent items, so they can refer
> only to a section of an existing extent (or the whole extent).

To me, NOCOW/PREALLOC is really the exception, not following all the
existing definition for all the OE/extent map definitions, even it's
only for transient NOCOW/PREALLOC OE.

And I really hope to remove the exception in the near future, even it
may mean some more complex OE spliting etc.

>
> I don't see how this rant is relevant to have here, as the patchset is
> about extent maps and not ordered extents.
> I would leave it out, as it's a separate thing and doesn't affect anything here.

Sure, I can leave it out for now.

>
> More comments inlined below.
>
>>
[...]
>>
>>          if (type != BTRFS_ORDERED_NOCOW) {
>> -               em = create_io_em(inode, start, len, block_start,
>> +               em = create_io_em(inode, start, len,
>>                                    orig_block_len, ram_bytes,
>>                                    BTRFS_COMPRESS_NONE, /* compress_type */
>>                                    file_extent, type);
>>                  if (IS_ERR(em))
>>                          goto out;
>>          }
>> +
>> +       /*
>> +        * NOTE: I know the numbers are totally wrong for NOCOW/PREALLOC,
>> +        * but it doesn't cause problem at least for now.
>
> They are not wrong, they are what they must be.
>
> This is all about passing the right values to
> btrfs_alloc_ordered_extent(), which has nothing to do with extent
> maps.

I think it's pretty obvious that, only for NOCOW/PREALLOC that OE
members differs from their corresponding extent map.

Sure, it is not causing anything wrong (in fact, anything other than the
current behavior is going to cause a lot of problems).

But just for the sake of consistency, even it doesn't affect any on-disk
change, I still want to modify the call sites so that we can directly
pass the btrfs_file_extent item to OE allocation.
Even if it means other modification mostly inside OE splitting part.

For now I can remove/modify the comments, but I do not think this is
consistent.

>
>> +        *
>> +        * For regular writes, we would have file_extent->offset as 0,
>> +        * thus we really only need disk_bytenr, every other length
>> +        * (disk_num_bytes/ram_bytes) would match @len and fe->num_bytes.
>> +        * The current numbers are totally fine.
>> +        *
>> +        * For NOCOW, we don't really care about the numbers except @file_pos
>
> I don't see any variable named @file_pos anywhere in this function.
>
>> +        * and @num_bytes, as we won't insert a file extent item at all.
>
> There's no @num_bytes either, there's a @len however.
>
>> +        *
>> +        * For PREALLOC, we do not use ordered extent's member, but
>
> ordered extent's member -> ordered extent members
>
>> +        * btrfs_mark_extent_written() would handle everything.
>
> would handle -> handles
>
>> +        *
>> +        * So here we intentionally go with pseudo numbers for the NOCOW/PREALLOC
>> +        * OEs, or btrfs_extract_ordered_extent() would need a completely new
>> +        * routine to handle NOCOW/PREALLOC splits, meanwhile result nothing
>> +        * different.
>> +        */
>
> I would just leave the entire comment out.
>
>>          ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
>> -                                            block_start, len, 0,
[...]
>> @@ -1778,7 +1778,9 @@ static void btrfs_rewrite_logical_zoned(struct btrfs_ordered_extent *ordered,
>>          write_lock(&em_tree->lock);
>>          em = search_extent_mapping(em_tree, ordered->file_offset,
>>                                     ordered->num_bytes);
>> -       em->block_start = logical;
>> +       /* The em should be a new COW extent, thus it should not has offset. */
>
> not has offset -> not have an offset
>
> Otherwise it seems fine, but I still want to go over the rest of the patchset.
> I'm going slowly over it, and after commenting on each inidividual
> patch, I'll comment on the cover letter.

Really appreciate the review so far.
And considering the patchset is not urgent, just take your time so that
I can also address the comments at the same time.

Thanks,
Qu
>
> Thanks.
>
>> +       ASSERT(em->offset == 0);
>> +       em->disk_bytenr = logical;
>>          free_extent_map(em);
>>          write_unlock(&em_tree->lock);
>>   }
>> diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
>> index 3743719d13f2..89b2b66e5bc0 100644
>> --- a/include/trace/events/btrfs.h
>> +++ b/include/trace/events/btrfs.h
>> @@ -291,7 +291,6 @@ TRACE_EVENT_CONDITION(btrfs_get_extent,
>>                  __field(        u64,  ino               )
>>                  __field(        u64,  start             )
>>                  __field(        u64,  len               )
>> -               __field(        u64,  block_start       )
>>                  __field(        u32,  flags             )
>>                  __field(        int,  refs              )
>>          ),
>> @@ -301,18 +300,16 @@ TRACE_EVENT_CONDITION(btrfs_get_extent,
>>                  __entry->ino            = btrfs_ino(inode);
>>                  __entry->start          = map->start;
>>                  __entry->len            = map->len;
>> -               __entry->block_start    = map->block_start;
>>                  __entry->flags          = map->flags;
>>                  __entry->refs           = refcount_read(&map->refs);
>>          ),
>>
>>          TP_printk_btrfs("root=%llu(%s) ino=%llu start=%llu len=%llu "
>> -                 "block_start=%llu(%s) flags=%s refs=%u",
>> +                 "flags=%s refs=%u",
>>                    show_root_type(__entry->root_objectid),
>>                    __entry->ino,
>>                    __entry->start,
>>                    __entry->len,
>> -                 show_map_type(__entry->block_start),
>>                    show_map_flags(__entry->flags),
>>                    __entry->refs)
>>   );
>> @@ -2608,7 +2605,6 @@ TRACE_EVENT(btrfs_extent_map_shrinker_remove_em,
>>                  __field(        u64,    root_id         )
>>                  __field(        u64,    start           )
>>                  __field(        u64,    len             )
>> -               __field(        u64,    block_start     )
>>                  __field(        u32,    flags           )
>>          ),
>>
>> @@ -2617,15 +2613,12 @@ TRACE_EVENT(btrfs_extent_map_shrinker_remove_em,
>>                  __entry->root_id        = inode->root->root_key.objectid;
>>                  __entry->start          = em->start;
>>                  __entry->len            = em->len;
>> -               __entry->block_start    = em->block_start;
>>                  __entry->flags          = em->flags;
>>          ),
>>
>> -       TP_printk_btrfs(
>> -"ino=%llu root=%llu(%s) start=%llu len=%llu block_start=%llu(%s) flags=%s",
>> +       TP_printk_btrfs("ino=%llu root=%llu(%s) start=%llu len=%llu flags=%s",
>>                          __entry->ino, show_root_type(__entry->root_id),
>>                          __entry->start, __entry->len,
>> -                       show_map_type(__entry->block_start),
>>                          show_map_flags(__entry->flags))
>>   );
>>
>> --
>> 2.45.0
>>
>>
>