Re: [PATCH 3/7] ext2: Avoid DAX zeroing to corrupt data

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Mon, 2016-05-16 at 17:22 +0200, Jan Kara wrote:
> On Thu 12-05-16 12:45:22, Ross Zwisler wrote:
> > 
> > On Wed, May 11, 2016 at 11:58:49AM +0200, Jan Kara wrote:
> > > 
> > > Currently ext2 zeroes any data blocks allocated for DAX inode
> > > however it
> > > still returns them as BH_New. Thus DAX code zeroes them again in
> > > dax_insert_mapping() which can possibly overwrite the data that
> > > has been
> > > already stored to those blocks by a racing dax_io(). Avoid
> > > marking
> > > pre-zeroed buffers as new.
> > > 
> > > Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> > >  fs/ext2/inode.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> > > index 6bd58e6ff038..1f07b758b968 100644
> > > --- a/fs/ext2/inode.c
> > > +++ b/fs/ext2/inode.c
> > > @@ -745,11 +745,11 @@ static int ext2_get_blocks(struct inode
> > > *inode,
> > >  			mutex_unlock(&ei->truncate_mutex);
> > >  			goto cleanup;
> > >  		}
> > > -	}
> > > +	} else
> > > +		set_buffer_new(bh_result);
> > >  
> > >  	ext2_splice_branch(inode, iblock, partial,
> > > indirect_blks, count);
> > >  	mutex_unlock(&ei->truncate_mutex);
> > > -	set_buffer_new(bh_result);
> > >  got_it:
> > >  	map_bh(bh_result, inode->i_sb, le32_to_cpu(chain[depth-
> > > 1].key));
> > >  	if (count > blocks_to_boundary)
> > > -- 
> > > 2.6.6
> > Interestingly this change is causing a bunch of xfstests
> > regressions for me
> > with ext2 + DAX.  All of these tests pass without this one change.
> Good catch. Attached patch fixes this issue for me. Preferably it
> should be
> merged before the above ext2 change.
> 
> 								Honza

Hey Jan,

In my patch 3 of the error handling series, I have:

-               err = dax_clear_sectors(inode->i_sb->s_bdev,
-                               le32_to_cpu(chain[depth-1].key) <<
-                               (inode->i_blkbits - 9),
-                               1 << inode->i_blkbits);
+               err = sb_issue_zeroout(inode->i_sb,
+                               le32_to_cpu(chain[depth-1].key), 1, GFP_NOFS);

Does this mean I have to change to send the sb_issue_zeroout for
'count' blocks.. i.e.

-               err = dax_clear_sectors(inode->i_sb->s_bdev,
-                               le32_to_cpu(chain[depth-1].key) <<
-                               (inode->i_blkbits - 9),
-                               1 << inode->i_blkbits);
+               err = sb_issue_zeroout(inode->i_sb,
+                               le32_to_cpu(chain[depth-1].key), count, GFP_NOFS);

If so, I'll update my series tomorrow to include in both of these changes.

Thanks,
	-Vishal