All of lore.kernel.org
 help / color / mirror / Atom feed
* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.