gfs2 iomap: BUG_ON(buffer_unmapped) in submit_bh_wbc

Commit Message

Andreas Gruenbacher May 24, 2018, 6:05 p.m. UTC

Hi Christoph,

I'm running into the following problem with the GFS2 iomap patches:

In order to get iomap_to_bh to set the New flag of bhs for newly
allocated blocks in __block_write_begin_int, I set iomap->type to
IOMAP_UNWRITTEN when allocating blocks. This has the side effect of
setting the Unwritten bh flag as well. GFS2 doesn't have the concept
of unwritten extents, so it doesn't use the Unwritten flag itself.
There's also nothing in gfs2 that would clear that flag.

When I write to a file on that filesystem with something like:

  xfs_io -t -f -c "pwrite -S 0x5a 2048 2048" -c "fsync" /mnt/scratch/foo

I run into a BUG_ON(buffer_unwritten(bh)) in __mpage_writepage ->
__block_write_full_page -> submit_bh_wbc. That BUG_ON was added in
commit 8fb0e34248 by Aneesh. So clearly the VFS expects the unwritten
flags to be cleared by the filesystem somewhere.

I can think of two ways to fix this:

(1) make iomap_to_bh set the buffer_new flag for IOMAP_MAPPED mappings
with the IOMAP_F_NEW flag set as well and stop abusing the Unwritten
bh flag,

(2) clear the Unwritten flags in the filesystem.


Approach (1) is quite trivial:

                bh->b_blocknr = (iomap->addr + offset - iomap->offset) >>
                                inode->i_blkbits;


If you think (2) is preferable, then where and when should the
filesystem clear the Unwritten bh flags?


Thanks,
Andreas

Comments

Christoph Hellwig May 24, 2018, 6:13 p.m. UTC | #1

On Thu, May 24, 2018 at 08:05:03PM +0200, Andreas Gruenbacher wrote:
> Approach (1) is quite trivial:
> 
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1908,7 +1908,7 @@ iomap_to_bh(struct inode *inode, sector_t,
> struct buffer_head *bh,
>                 set_buffer_unwritten(bh);
>                 /* FALLTHRU */
>         case IOMAP_MAPPED:
> -               if (offset >= i_size_read(inode))
> +               if ((iomap->flags & IOMAP_F_NEW) || offset >=
> i_size_read(inode))
>                         set_buffer_new(bh);
>                 bh->b_blocknr = (iomap->addr + offset - iomap->offset) >>
>                                 inode->i_blkbits;
> 

This is the right thing to do.  We just didn't need to do it for XFS
as we'll never allocate normal blocks in write_begin.

You'll probably need similar tweaks in my new iomap write code, which
you're hopefully lookin into as well.

Andreas Gruenbacher May 24, 2018, 6:25 p.m. UTC | #2

On 24 May 2018 at 20:13, Christoph Hellwig <[e-mail]> wrote:
> On Thu, May 24, 2018 at 08:05:03PM +0200, Andreas Gruenbacher wrote:
>> Approach (1) is quite trivial:
>>
>> --- a/fs/buffer.c
>> +++ b/fs/buffer.c
>> @@ -1908,7 +1908,7 @@ iomap_to_bh(struct inode *inode, sector_t,
>> struct buffer_head *bh,
>>                 set_buffer_unwritten(bh);
>>                 /* FALLTHRU */
>>         case IOMAP_MAPPED:
>> -               if (offset >= i_size_read(inode))
>> +               if ((iomap->flags & IOMAP_F_NEW) || offset >=
>> i_size_read(inode))
>>                         set_buffer_new(bh);
>>                 bh->b_blocknr = (iomap->addr + offset - iomap->offset) >>
>>                                 inode->i_blkbits;
>>
>
> This is the right thing to do.  We just didn't need to do it for XFS
> as we'll never allocate normal blocks in write_begin.

Okay, that's easy.

> You'll probably need similar tweaks in my new iomap write code, which
> you're hopefully looking into as well.

Didn't get very far with your bh removal patches so far.

Thanks,
Andreas

Christoph Hellwig May 25, 2018, 12:24 p.m. UTC | #3

On Thu, May 24, 2018 at 08:25:55PM +0200, Andreas Gruenbacher wrote:
> Didn't get very far with your bh removal patches so far.

I think we could probably lift the whole XFS writeback code to iomap.c
once it stabilizes a little.  It just needs to use struct iomap
instead of the xfs-specific xfs_bmbt_irec, and the file system would
have to provide the map_blocks and end_io hooks.

Patch

--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1908,7 +1908,7 @@  iomap_to_bh(struct inode *inode, sector_t,
struct buffer_head *bh,
                set_buffer_unwritten(bh);
                /* FALLTHRU */
        case IOMAP_MAPPED:
-               if (offset >= i_size_read(inode))
+               if ((iomap->flags & IOMAP_F_NEW) || offset >=
i_size_read(inode))
                        set_buffer_new(bh);