Re: [PATCH 5/5] update i_disksize coherently with block allocation

[Dmitry Monakhov <dmonakhov@openvz.org>]

On Mon, 25 Aug 2014 21:13:10 -0400, Theodore Ts'o <tytso@mit.edu> wrote:
> On Fri, Aug 22, 2014 at 03:32:27PM +0400, Dmitry Monakhov wrote:
> > Writeback call trace looks like follows:
> > ext4_writepages
> >  while(nr_pages)
> >  ->journal_start
> >  ->mpage_map_and_submit_extent -> may alloc some blocks
> >    ->mpage_map_one_extent
> >  ->journal_stop
> > In case of delalloc block i_disksize may be less than i_size. So we have to
> > update i_disksize each time we allocated and submitted some blocks beyond
> > i_disksize. And we MUST update it in the same transaction, otherwise this
> > result in fs-inconsistency in case of upcoming power-failure.
> 
> This description doesn't seem to be consistent with the change in the
> patch; the patch included makes sure that on an error, we update the
> on-disk block size if some blocks had been previously allocated.
> Which looks OK, but the commit description seems to imply tha the
> patch does more than this.

>  Did part of the patch get dropped, or
> should we rewrite the commit description to be more clear what it is
> changing?
No. patch was written like this from very beginning. 
By logic is follows. We must update i_disksize in the same transaction
Before the patch i_disksize was updated only if requested range was fully completed.
But we also have to update it in case of error. And patch fix what.
> > Another possible way to fix that issue is to insert inode to orhphan list
> > on ext4_writepages entrance.
> > 
> > testcase: xfstest generic/019
> > 
> > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> > ---
> >  fs/ext4/inode.c |   10 ++++++++--
> >  1 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index 3919e25..b1d92fb 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -2077,6 +2077,7 @@ static int mpage_map_and_submit_extent(handle_t *handle,
> >  	struct ext4_map_blocks *map = &mpd->map;
> >  	int err;
> >  	loff_t disksize;
> > +	int progress = 0;
> >  
> >  	mpd->io_submit.io_end->offset =
> >  				((loff_t)map->m_lblk) << inode->i_blkbits;
> > @@ -2093,8 +2094,11 @@ static int mpage_map_and_submit_extent(handle_t *handle,
> >  			 * is non-zero, a commit should free up blocks.
> >  			 */
> >  			if ((err == -ENOMEM) ||
> > -			    (err == -ENOSPC && ext4_count_free_clusters(sb)))
> > +			    (err == -ENOSPC && ext4_count_free_clusters(sb))) {
> > +				if (progress)
> > +					goto update_disksize;
> >  				return err;
> > +			}
> >  			ext4_msg(sb, KERN_CRIT,
> >  				 "Delayed block allocation failed for "
> >  				 "inode %lu at logical offset %llu with"
> > @@ -2111,15 +2115,17 @@ static int mpage_map_and_submit_extent(handle_t *handle,
> >  			*give_up_on_write = true;
> >  			return err;
> >  		}
> > +		progress = 1;
> >  		/*
> >  		 * Update buffer state, submit mapped pages, and get us new
> >  		 * extent to map
> >  		 */
> >  		err = mpage_map_and_submit_buffers(mpd);
> >  		if (err < 0)
> > -			return err;
> > +			goto update_disksize;
> >  	} while (map->m_len);
> >  
> > +update_disksize:
> >  	/*
> >  	 * Update on-disk size after IO is submitted.  Races with
> >  	 * truncate are avoided by checking i_size under i_data_sem.
> > -- 
> > 1.7.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html