Re: [PATCH v7 00/39] btrfs: zoned block device support

[Naohiro Aota <Naohiro.Aota@xxxxxxx>]

On Wed, Sep 16, 2020 at 05:42:50PM +0000, Johannes Thumshirn wrote:
> On 15/09/2020 10:25, David Sterba wrote:
> > On Fri, Sep 11, 2020 at 09:32:20PM +0900, Naohiro Aota wrote:
> > > Changelog
> > > v6:
> > >  - Use bitmap helpers (Johannes)
> > >  - Code cleanup (Johannes)
> > >  - Rebased on kdave/for-5.5
> > >  - Enable the tree-log feature.
> > >  - Treat conventional zones as sequential zones, so we can now allow
> > >    mixed allocation of conventional zone and sequential write required
> > >    zone to construct a block group.
> > >  - Implement log-structured superblock
> > >    - No need for one conventional zone at the beginning of a device.
> > >  - Fix deadlock of direct IO writing
> > >  - Fix building with !CONFIG_BLK_DEV_ZONED (Johannes)
> > >  - Fix leak of zone_info (Johannes)
> >
> > I did a quick check to see if the patchset passes the default VM tests
> > and there's use after free short after the fstests start. No zoned
> > devices or such. I had to fix some conflicts when rebasing on misc-next
> > but I tried to base it on the last iomap-dio patch ("btrfs: switch to
> > iomap for direct IO"), same result so it's something in the zoned
> > patches.
> >
> > The reported pointer 0x6b6b6b6b6d1918eb contains the use-after-free
> > poison (0x6b) (CONFIG_PAGE_POISONING=y).
> >
> > MKFS_OPTIONS  -- -f -K --csum xxhash /dev/vdb
> > MOUNT_OPTIONS -- -o discard /dev/vdb /tmp/scratch
>
> Hi David,
>
> Can you check if this on top of the series fixes the issue? According
> to Keith we can't call bio_iovec() from endio() as the iterator is already
> advanced (see req_bio_endio()).

Thank you for fixing this.

> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index bda4e02b5eab..311956697682 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2753,10 +2753,6 @@ static void end_bio_extent_writepage(struct bio *bio)
>        u64 end;
>        struct bvec_iter_all iter_all;
>
> -       btrfs_record_physical_zoned(bio_iovec(bio).bv_page->mapping->host,
> -                                   page_offset(bio_iovec(bio).bv_page) + bio_iovec(bio).bv_offset,
> -                                   bio);
> -
>        ASSERT(!bio_flagged(bio, BIO_CLONED));
>        bio_for_each_segment_all(bvec, bio, iter_all) {
>                struct page *page = bvec->bv_page;
> @@ -2782,6 +2778,7 @@ static void end_bio_extent_writepage(struct bio *bio)
>                start = page_offset(page);
>                end = start + bvec->bv_offset + bvec->bv_len - 1;
>
> +               btrfs_record_physical_zoned(inode, start, bio);

We need to record the physical address only once per an ordered extent.
So, this should be like:

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index c21d1dbe314e..0bbe6e52ea0d 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2748,6 +2748,7 @@ static void end_bio_extent_writepage(struct bio *bio)
        u64 start;
        u64 end;
        struct bvec_iter_all iter_all;
+       bool first_bvec = true;

        ASSERT(!bio_flagged(bio, BIO_CLONED));
        bio_for_each_segment_all(bvec, bio, iter_all) {
@@ -2774,6 +2775,11 @@ static void end_bio_extent_writepage(struct bio *bio)
                start = page_offset(page);
                end = start + bvec->bv_offset + bvec->bv_len - 1;

+               if (first_bvec) {
+                       btrfs_record_physical_zoned(inode, start, bio);
+                       first_bvec = false;
+               }
+
                end_extent_writepage(page, error, start, end);
                end_page_writeback(page);
        }


>                end_extent_writepage(page, error, start, end);
>                end_page_writeback(page);
>        }
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 576f8e333f16..6fdb21029ea9 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -1086,8 +1086,7 @@ void btrfs_record_physical_zoned(struct inode *inode, u64 file_offset,
> {
>        struct btrfs_ordered_extent *ordered;
>        struct bio_vec bvec = bio_iovec(bio);
> -       u64 physical = ((u64)bio->bi_iter.bi_sector << SECTOR_SHIFT) +
> -               bvec.bv_offset;
> +       u64 physical = (u64)bio->bi_iter.bi_sector << SECTOR_SHIFT;
>
>        if (bio_op(bio) != REQ_OP_ZONE_APPEND)
>                return;