Re: [PATCH] xfs: remove XFS_TRANS_RESERVE in collapse range

[Dave Chinner <david@fromorbit.com>]

On Thu, May 08, 2014 at 09:36:37AM -0400, Brian Foster wrote:
> On Thu, May 08, 2014 at 06:49:14PM +0900, Namjae Jeon wrote:
> > There is no need to dip into reserve pool. Reserve pool is used for much
> > more important things. And xfs_trans_reserve will never return ENOSPC
> > because punch hole is already done. If we get ENOSPC, collapse range
> > will be simply failed.
> > 
> > Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
> > Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com>
> > ---
> >  fs/xfs/xfs_bmap_util.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index 296160b..91a43c5 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -1519,7 +1519,6 @@ xfs_collapse_file_space(
> >  
> >  	while (!error && !done) {
> >  		tp = xfs_trans_alloc(mp, XFS_TRANS_DIOSTRAT);
> > -		tp->t_flags |= XFS_TRANS_RESERVE;
> 
> Makes sense.
> 
> >  		/*
> >  		 * We would need to reserve permanent block for transaction.
> >  		 * This will come into picture when after shifting extent into
> > @@ -1529,7 +1528,7 @@ xfs_collapse_file_space(
> >  		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_write,
> >  				XFS_DIOSTRAT_SPACE_RES(mp, 0), 0);
> >  		if (error) {
> > -			ASSERT(error == ENOSPC || XFS_FORCED_SHUTDOWN(mp));
> > +			ASSERT(XFS_FORCED_SHUTDOWN(mp));
> 
> The xfs_trans_reserve() call still reserves XFS_DIOSTRAT_SPACE_RES()
> blocks, so therefore I think ENOSPC is still a possibility. The question
> is probably whether or not we need to reserve blocks for this
> transaction.
> 
> Making a pass through the code... we have the possibility of deleting a
> btree record in xfs_bmap_shift_extents(). This in turn could potentially
> free a btree block, which frees space. I _think_ this could mean we
> want to keep the block reservation because we update the free space
> trees, but I suppose that could be handled by the freelist...
> 
> Perhaps Dave can confirm which direction we should go here..?

Having collapse range fail with ENOSPC is not an issue - it is being
executed in a context where we can fail safely and return an error
to the user.

XFS_TRANS_RESERVE is used in places where a failure is unrecoverable
or there is no one to report the error to.  e.g. prevent data loss
due to ENOSPC in unwritten extent conversion during background
buffered write IO completion

So here there is no need for it at all....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs