* [PATCH] xfs: remove XFS_TRANS_RESERVE in collapse range @ 2014-05-08 9:49 Namjae Jeon 2014-05-08 13:36 ` Brian Foster 0 siblings, 1 reply; 6+ messages in thread From: Namjae Jeon @ 2014-05-08 9:49 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs 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; /* * 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)); xfs_trans_cancel(tp, 0); break; } -- 1.7.11-rc0 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: remove XFS_TRANS_RESERVE in collapse range 2014-05-08 9:49 [PATCH] xfs: remove XFS_TRANS_RESERVE in collapse range Namjae Jeon @ 2014-05-08 13:36 ` Brian Foster 2014-05-08 21:17 ` Dave Chinner 0 siblings, 1 reply; 6+ messages in thread From: Brian Foster @ 2014-05-08 13:36 UTC (permalink / raw) To: Namjae Jeon; +Cc: xfs 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..? Brian > xfs_trans_cancel(tp, 0); > break; > } > -- > 1.7.11-rc0 > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: remove XFS_TRANS_RESERVE in collapse range 2014-05-08 13:36 ` Brian Foster @ 2014-05-08 21:17 ` Dave Chinner 2014-05-08 21:51 ` Brian Foster 0 siblings, 1 reply; 6+ messages in thread From: Dave Chinner @ 2014-05-08 21:17 UTC (permalink / raw) To: Brian Foster; +Cc: Namjae Jeon, xfs 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: remove XFS_TRANS_RESERVE in collapse range 2014-05-08 21:17 ` Dave Chinner @ 2014-05-08 21:51 ` Brian Foster 2014-05-08 22:05 ` Dave Chinner 0 siblings, 1 reply; 6+ messages in thread From: Brian Foster @ 2014-05-08 21:51 UTC (permalink / raw) To: Dave Chinner; +Cc: Namjae Jeon, xfs On Fri, May 09, 2014 at 07:17:12AM +1000, Dave Chinner wrote: > 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.... > Yeah, dropping XFS_TRANS_RESERVE makes sense. The question I have is whether we should reserve blocks for this transaction (for a potential bmbt block free). If we do reserve blocks, then I assume the ENOSPC assert should stick around. Brian > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: remove XFS_TRANS_RESERVE in collapse range 2014-05-08 21:51 ` Brian Foster @ 2014-05-08 22:05 ` Dave Chinner 2014-05-12 23:53 ` Namjae Jeon 0 siblings, 1 reply; 6+ messages in thread From: Dave Chinner @ 2014-05-08 22:05 UTC (permalink / raw) To: Brian Foster; +Cc: Namjae Jeon, xfs On Thu, May 08, 2014 at 05:51:16PM -0400, Brian Foster wrote: > On Fri, May 09, 2014 at 07:17:12AM +1000, Dave Chinner wrote: > > 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.... > > > > Yeah, dropping XFS_TRANS_RESERVE makes sense. The question I have is > whether we should reserve blocks for this transaction (for a potential > bmbt block free). If we do reserve blocks, then I assume the ENOSPC > assert should stick around. Yes, the assert is wrong because xfs_trans_reserve() can return ENOMEM as well as ENOSPC. It should just be removed. Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] xfs: remove XFS_TRANS_RESERVE in collapse range 2014-05-08 22:05 ` Dave Chinner @ 2014-05-12 23:53 ` Namjae Jeon 0 siblings, 0 replies; 6+ messages in thread From: Namjae Jeon @ 2014-05-12 23:53 UTC (permalink / raw) To: 'Dave Chinner'; +Cc: 'Brian Foster', xfs > > On Thu, May 08, 2014 at 05:51:16PM -0400, Brian Foster wrote: > > On Fri, May 09, 2014 at 07:17:12AM +1000, Dave Chinner wrote: > > > 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.... > > > > > > > Yeah, dropping XFS_TRANS_RESERVE makes sense. The question I have is > > whether we should reserve blocks for this transaction (for a potential > > bmbt block free). If we do reserve blocks, then I assume the ENOSPC > > assert should stick around. > > Yes, the assert is wrong because xfs_trans_reserve() can > return ENOMEM as well as ENOSPC. It should just be removed. Okay, I will remove it on v2 patch. Thanks Dave and Brian! > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-05-12 23:53 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-05-08 9:49 [PATCH] xfs: remove XFS_TRANS_RESERVE in collapse range Namjae Jeon 2014-05-08 13:36 ` Brian Foster 2014-05-08 21:17 ` Dave Chinner 2014-05-08 21:51 ` Brian Foster 2014-05-08 22:05 ` Dave Chinner 2014-05-12 23:53 ` Namjae Jeon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox