Message-ID: <20080315075107.GA7248@skywalker>
Date:	Sat, 15 Mar 2008 13:21:07 +0530
From:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
To:	Mingming Cao <cmm@...ibm.com>
Cc:	tytso@....edu, adilger@....com, jack@...e.cz,
	linux-ext4@...r.kernel.org
Subject: Re: [PATCH] ext4: Fail migrate if we allocated new blocks via mmap
	write.

On Fri, Mar 14, 2008 at 12:08:57PM -0700, Mingming Cao wrote:
> On Fri, 2008-03-14 at 12:34 +0530, Aneesh Kumar K.V wrote:

....

> > > > +	if (retval > 0) {
> > > > +		/*
> > > > +		 * We allocated new blocks which will result in i_data
> > > > +		 * format to change.  Force the migrate to fail by
> > > > +		 * clearing migrate flags
> > > > +		 */
> > > > +		EXT4_I(inode)->i_flags = EXT4_I(inode)->i_flags &
> > > > +							~EXT4_EXT_MIGRATE;
> > > > +	}
> > > 
> > > We probably need to check buffer_new() for the resulting bh, as retval >
> > > 0 doesn't necessarily means ext4_ext_get_blocks() allocated new blocks.
> > 
> > 
> > Only if we request with create = 0 the API returns >0 and buffer head
> > unmapped.
> > 
> 
> But buffer_mapped(bh) doesn't necessarily mean buffer_new(bh) is true
> 
> In a race allocation case, it's possible that after re-grab the write
> lock of the i_data_sem, the blocks in range has already been allocated
> by other mmaped write to the same range. It's a minor optimization to
> avoid clearing the flag if there is no allocation, though, but it's more
> clear to check the buffer_new() flag here.



I added retval > 0 && buffer_new(bh). I also moved the check only for
ext3 inode type.

> 
> > > 
> > > And I think this check should only for ext3 type files, maybe checking
> > > the flag or move the "if" right after ext4_get_blocks_handle()?
> > > 
> > > >  	up_write((&EXT4_I(inode)->i_data_sem));
> > > >  	return retval;
> > > >  }
> > > > @@ -2962,7 +2972,8 @@ static int ext4_do_update_inode(handle_t *handle,
> > > >  	if (ext4_inode_blocks_set(handle, raw_inode, ei))
> > > >  		goto out_brelse;
> > > >  	raw_inode->i_dtime = cpu_to_le32(ei->i_dtime);
> > > > -	raw_inode->i_flags = cpu_to_le32(ei->i_flags);
> > > > +	/* clear the migrate flag in the raw_inode */
> > > > +	raw_inode->i_flags = cpu_to_le32(ei->i_flags & ~EXT4_EXT_MIGRATE);
> > > 
> > > Do we need to save this flag on-disk?
> > 
> > 
> > We don't need to. That's why i am clearing it in the raw_inode. We still
> > need to have it in ext4_inode_info so that an ongoing migrate doesn't
> > fail.
> > 
> Oh, I mean "clear" this flag...it seems to me that doing this update for
> every on-disk inode update is unnecessary. Probably just clearing this
> flag at read_inode() time when the inode first load() from disk and only
> keep this flag around in the in-core memory?
> 
> > > 
> > > >  	if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
> > > >  	    cpu_to_le32(EXT4_OS_HURD))
> > > >  		raw_inode->i_file_acl_high =
> > > > @@ -3502,9 +3513,5 @@ int ext4_page_mkwrite(struct vm_area_struct *vma, struct page *page)
> > > >  	 * access and zero out the page. The journal handle get initialized
> > > >  	 * in ext4_get_block.
> > > >  	 */
> > > > -	/* FIXME!! should we take inode->i_mutex ? Currently we can't because
> > > > -	 * it has a circular locking dependency with DIO. But migrate expect
> > > > -	 * i_mutex to ensure no i_data changes
> > > > -	 */
> > > >  	return block_page_mkwrite(vma, page, ext4_get_block);
> > > 
> > > If you update this patch, how about split this part to a separate fix
> > > and merge that with it's parent ext4-page-mkwrite() patch? 
> > > 


removed this comment from ext4-page-mkwrite patch



> > > >  }
> > > > diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> > > > index 5c1e27d..f4c9e78 100644
> > > > --- a/fs/ext4/migrate.c
> > > > +++ b/fs/ext4/migrate.c
> > > > @@ -327,7 +327,7 @@ static int free_ind_block(handle_t *handle, struct inode *inode, __le32 *i_data)
> > > >  }
> > > > 
> > > >  static int ext4_ext_swap_inode_data(handle_t *handle, struct inode *inode,
> > > > -				struct inode *tmp_inode)
> > > > +						struct inode *tmp_inode)
> > > >  {
> > > >  	int retval;
> > > >  	__le32	i_data[3];
> > > > @@ -351,6 +351,18 @@ static int ext4_ext_swap_inode_data(handle_t *handle, struct inode *inode,
> > > > 
> > > >  	down_write(&EXT4_I(inode)->i_data_sem);
> > > >  	/*
> > > > +	 * if EXT4_EXT_MIGRATE is cleared a block allocation
> > > > +	 * happened after we started the migrate. We need to
> > > > +	 * fail the migrate
> > > > +	 */
> > > > +	if (!(EXT4_I(inode)->i_flags & EXT4_EXT_MIGRATE)) {
> > > > +		retval = -EAGAIN;
> > > > +		up_write(&EXT4_I(inode)->i_data_sem);
> > > > +		goto err_out;
> > > > +	} else
> > > > +		EXT4_I(inode)->i_flags = EXT4_I(inode)->i_flags &
> > > > +							~EXT4_EXT_MIGRATE;
> > > > +	/*
> > > 
> > > I could not see the caller of ext4_ext_swap_inode_data():
> > > ext4_ext_mirgrate() checks the return value from
> > > ext4_ext_swap_inode_data(). We probably should free allocated blocks,
> > > rebuild the extents tree for the tmp inode and do the swap again in the
> > > EAGAIN case. And for other error case probably need proper error
> > > handling too.
> > 
> > The ioctl will return EAGAIN and the application can issue the ioctl
> > again.
> > 
> In that case, I assume a new tmp inode is created and new blocks will be
> allocated? What I am refereing is the old tmp inode and the allocated
> blocks for it should be freed in case of EAGAIN error...I don't see the
> code is handling that. Maybe I missed something?

I updated the patch to call free_ext_block if ext4_ext_swap_inode_data
failed. I also verified that we have block and inode accounting correct
by running e2fsck and debugfs:stats.

I am attaching the updated patch below.

View attachment "ext4-page-mkwrite.patch" of type "text/x-diff" (3079 bytes)

View attachment "ext4-migration-locking-fix.patch" of type "text/x-diff" (5287 bytes)

Powered by blists - more mailing lists