Possible bug with extent status tree

Possible bug with extent status tree

[Jan Kara]

  Hello,

  while looking into the ext4 code I spotted one thing which I think is a
bug introduced by extent status tree code. The problem is that
ext4_map_blocks() checks extent status tree and if the extent is found, it
doesn't call into ext4_ext_map_blocks(). However ext4_ext_direct_IO() expects
that if the extent DIO is done to is unwritten, EXT4_IO_END_UNWRITTEN flag
gets set in the io_end (or inode) flags and that happens only in
ext4_ext_map_blocks().

The easiest fix seems to be to move setting of flags from
ext4_ext_map_blocks() up into ext4_map_blocks() (or maybe even
_ext4_get_block()). What do you think?

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: Possible bug with extent status tree

[Zheng Liu]

On Mon, Mar 11, 2013 at 09:38:34PM +0100, Jan Kara wrote:
>   Hello,
> 
>   while looking into the ext4 code I spotted one thing which I think is a
> bug introduced by extent status tree code. The problem is that
> ext4_map_blocks() checks extent status tree and if the extent is found, it
> doesn't call into ext4_ext_map_blocks(). However ext4_ext_direct_IO() expects
> that if the extent DIO is done to is unwritten, EXT4_IO_END_UNWRITTEN flag
> gets set in the io_end (or inode) flags and that happens only in
> ext4_ext_map_blocks().
> 
> The easiest fix seems to be to move setting of flags from
> ext4_ext_map_blocks() up into ext4_map_blocks() (or maybe even
> _ext4_get_block()). What do you think?

Hi Jan,

Thanks for reviewing the code.  But I don't think it is a potential bug.
Let's see what happens after adding extent status tree.  There are three
cases that we need to take care, a) no block has been allocated, b)
unwritten extent has been allocated, and c) written extent has been
allocated.

a) no block has been allocated.
In this case, both extent tree on disk and status tree in memory haven't
this extent.  When we do a extent-based dio write, ext4_get_block_write
will be called with EXT4_GET_BLOCKS_IO_CREATE_EXT flag.  In
ext4_map_blocks(), the codepath looks like:

ext4_map_blocks()
  ->ext4_es_lookup_extent()
    [no extent found in status tree]
  ->ext4_ext_map_blocks() without flags
    [no extent found in extent tree]
  ->ext4_ext_map_blocks() with EXT4_GET_BLOCKS_IO_CREATE_EXT
    ->ext4_mb_new_blocks()
      [allocate an unwritten extent]
    ->ext4_set_io_unwritten_flag()
      [set EXT4_IO_END_UNWRITTEN flag because _PRE_IO flag is set and an
       unwritten extent has been allocated]

In this case, we are safe.

b) unwritten extent has been allocated
In this case, both extent tree on disk and status tree in memory have an
unwritten extent, and codepath looks like:

ext4_map_blocks()
  ->ext4_es_lookup_extent()
    [unwritten extent found in status tree, EXT4_MAP_UNWRITTEN flag is
     marked in map->m_flags.  In the mean time, ext4_ext_map_blocks()
     isn't called]
  [But we don't return from ext4_map_blocks because EXT4_GET_BLOCKS_CREATE
   flag is set and EXT4_MAP_MAPPED flag is not set]
  ->ext4_ext_map_blocks() with EXT4_GET_BLOCKS_IO_CREATE_EXT
    ->ext4_ext_handle_uninitialized_extents()
      ->ext4_set_io_unwritten_flag()
        [set EXT4_IO_END_UNWRITTEN flag because _PRE_IO flag is set]

So it seems that there is no bug.

c) written extent has been allocated
In this case, both extent tree on disk and status tree in memory have an
written extent and codepath looks like:

ext4_map_blocks()
  ->ext4_es_lookup_extent()
    [written extent found in status tree, EXT4_MAP_MAPPED flag is set in
     map->m_flags.  In the mean time, ext4_ext_map_blocks() isn't called]
  [we return directly because retval > 0 and EXT4_MAP_MAPPED is set, and
   *EXT4_IO_END_UNWRITTEN flag is not set*]

I guess that you are aruging this case.  If we don't apply status tree
code, the codepath will look like:

ext4_map_blocks()
  ->ext4_ext_map_blocks() without flags
    [written extent found in extent tree.  We return directly because
     retval > 0 and EXT4_MAP_MAPPED is set.]

Thus, it seems we are safe.  Am I miss something?

Thanks,
                                                - Zheng

Re: Possible bug with extent status tree

[Jan Kara]

  Hello,

On Tue 12-03-13 11:53:19, Zheng Liu wrote:
> On Mon, Mar 11, 2013 at 09:38:34PM +0100, Jan Kara wrote:
> >   Hello,
> > 
> >   while looking into the ext4 code I spotted one thing which I think is a
> > bug introduced by extent status tree code. The problem is that
> > ext4_map_blocks() checks extent status tree and if the extent is found, it
> > doesn't call into ext4_ext_map_blocks(). However ext4_ext_direct_IO() expects
> > that if the extent DIO is done to is unwritten, EXT4_IO_END_UNWRITTEN flag
> > gets set in the io_end (or inode) flags and that happens only in
> > ext4_ext_map_blocks().
> > 
> > The easiest fix seems to be to move setting of flags from
> > ext4_ext_map_blocks() up into ext4_map_blocks() (or maybe even
> > _ext4_get_block()). What do you think?
> 
> Hi Jan,
> 
> Thanks for reviewing the code.  But I don't think it is a potential bug.
> Let's see what happens after adding extent status tree.  There are three
> cases that we need to take care, a) no block has been allocated, b)
> unwritten extent has been allocated, and c) written extent has been
> allocated.
> 
> a) no block has been allocated.
> In this case, both extent tree on disk and status tree in memory haven't
> this extent.  When we do a extent-based dio write, ext4_get_block_write
> will be called with EXT4_GET_BLOCKS_IO_CREATE_EXT flag.  In
> ext4_map_blocks(), the codepath looks like:
> 
> ext4_map_blocks()
>   ->ext4_es_lookup_extent()
>     [no extent found in status tree]
>   ->ext4_ext_map_blocks() without flags
>     [no extent found in extent tree]
>   ->ext4_ext_map_blocks() with EXT4_GET_BLOCKS_IO_CREATE_EXT
>     ->ext4_mb_new_blocks()
>       [allocate an unwritten extent]
>     ->ext4_set_io_unwritten_flag()
>       [set EXT4_IO_END_UNWRITTEN flag because _PRE_IO flag is set and an
>        unwritten extent has been allocated]
> 
> In this case, we are safe.
  Agreed.

> b) unwritten extent has been allocated
> In this case, both extent tree on disk and status tree in memory have an
> unwritten extent, and codepath looks like:
> 
> ext4_map_blocks()
>   ->ext4_es_lookup_extent()
>     [unwritten extent found in status tree, EXT4_MAP_UNWRITTEN flag is
>      marked in map->m_flags.  In the mean time, ext4_ext_map_blocks()
>      isn't called]
>   [But we don't return from ext4_map_blocks because EXT4_GET_BLOCKS_CREATE
>    flag is set and EXT4_MAP_MAPPED flag is not set]
>   ->ext4_ext_map_blocks() with EXT4_GET_BLOCKS_IO_CREATE_EXT
>     ->ext4_ext_handle_uninitialized_extents()
>       ->ext4_set_io_unwritten_flag()
>         [set EXT4_IO_END_UNWRITTEN flag because _PRE_IO flag is set]
> 
> So it seems that there is no bug.
  Ah, right. This is what I was missing. I didn't realize that we won't
set EXT4_MAP_MAPPED when the extent is unwritten so we will end up calling
ext4_ext_map_blocks() later. Sorry for the noise.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR