xfs: remove XFS_TRANS_RESERVE in collapse range
PATCH 1/1 xfs: remove XFS_TRANS_RESERVE in collapse range
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.
--- fs/xfs/xfs_bmap_util.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
fs/xfs/xfs_bmap_util.c +1 -2
@@ -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 ; }
Signature · 6 lines
1.7.11-rc0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 16 lines · Namjae Jeon
There is no need to dip into reserve pool. Reserve pool is used for much more important things. And
xfs_trans_reservewill never return ENOSPC because punch hole is already done. If we get ENOSPC, collapse range will be simply failed.Signed-off-by Namjae Jeon <[email protected]>Signed-off-by Ashish Sangwan <[email protected]>--- fs/xfs/xfs_bmap_util.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)fs/xfs/xfs_bmap_util.c +0
-0
fs/xfs/xfs_bmap_util.c xfs_collapse_file_space( · line 1522
Quoted 4 lines
fs/xfs/xfs_bmap_util.c +0
-1@@ -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.
fs/xfs/xfs_bmap_util.c xfs_collapse_file_space( · line 1531
Quoted 9 lines
fs/xfs/xfs_bmap_util.c +1
-1/* * 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
Quoted 10 lines
fs/xfs/xfs_bmap_util.c +0
-0xfs_trans_cancel (tp,0 );break ; }Signature · 6 lines
1.7.11-rc0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 39 lines · Brian Foster
Quoted 16 lines · Namjae Jeon
There is no need to dip into reserve pool. Reserve pool is used for much more important things. And
xfs_trans_reservewill never return ENOSPC because punch hole is already done. If we get ENOSPC, collapse range will be simply failed.Signed-off-by Namjae Jeon <[email protected]>Signed-off-by Ashish Sangwan <[email protected]>--- fs/xfs/xfs_bmap_util.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)fs/xfs/xfs_bmap_util.c +0
-0fs/xfs/xfs_bmap_util.c xfs_collapse_file_space( · line 1522
Quoted 4 lines
fs/xfs/xfs_bmap_util.c +0
-1@@ -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.
fs/xfs/xfs_bmap_util.c xfs_collapse_file_space( · line 1531
Quoted 9 lines
fs/xfs/xfs_bmap_util.c +1
-1/* * 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 reservesXFS_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.
Quoted 7 lines
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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 52 lines · Dave Chinner
Quoted 39 lines · Brian Foster
Quoted 16 lines · Namjae Jeon
There is no need to dip into reserve pool. Reserve pool is used for much more important things. And
xfs_trans_reservewill never return ENOSPC because punch hole is already done. If we get ENOSPC, collapse range will be simply failed.Signed-off-by Namjae Jeon <[email protected]>Signed-off-by Ashish Sangwan <[email protected]>--- fs/xfs/xfs_bmap_util.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)fs/xfs/xfs_bmap_util.c +0
-0fs/xfs/xfs_bmap_util.c xfs_collapse_file_space( · line 1522
Quoted 4 lines
fs/xfs/xfs_bmap_util.c +0
-1@@ -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.
fs/xfs/xfs_bmap_util.c xfs_collapse_file_space( · line 1531
Quoted 9 lines
fs/xfs/xfs_bmap_util.c +1
-1/* * 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 reservesXFS_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.Quoted 7 lines
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.
Quoted 7 lines
XFS_TRANS_RESERVEis 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 completionSo 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
Quoted 6 lines
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 58 lines · Brian Foster
Quoted 57 lines · Dave Chinner
Quoted 39 lines · Brian Foster
Quoted 16 lines · Namjae Jeon
There is no need to dip into reserve pool. Reserve pool is used for much more important things. And
xfs_trans_reservewill never return ENOSPC because punch hole is already done. If we get ENOSPC, collapse range will be simply failed.Signed-off-by Namjae Jeon <[email protected]>Signed-off-by Ashish Sangwan <[email protected]>--- fs/xfs/xfs_bmap_util.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)fs/xfs/xfs_bmap_util.c +0
-0fs/xfs/xfs_bmap_util.c xfs_collapse_file_space( · line 1522
Quoted 4 lines
fs/xfs/xfs_bmap_util.c +0
-1@@ -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.
fs/xfs/xfs_bmap_util.c xfs_collapse_file_space( · line 1531
Quoted 9 lines
fs/xfs/xfs_bmap_util.c +1
-1/* * 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 reservesXFS_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.Quoted 7 lines
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_RESERVEis 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
Quoted 7 lines
Quoted 2 lines
So here there is no need for it at all....
Yeah, dropping
XFS_TRANS_RESERVEmakes 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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 63 lines · Brian Foster
Quoted 61 lines · Dave Chinner
Quoted 59 lines · Brian Foster
Quoted 39 lines · Namjae Jeon
Quoted 16 lines
There is no need to dip into reserve pool. Reserve pool is used for much more important things. And
xfs_trans_reservewill never return ENOSPC because punch hole is already done. If we get ENOSPC, collapse range will be simply failed.Signed-off-by Namjae Jeon <[email protected]>Signed-off-by Ashish Sangwan <[email protected]>--- fs/xfs/xfs_bmap_util.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)fs/xfs/xfs_bmap_util.c +0
-0fs/xfs/xfs_bmap_util.c xfs_collapse_file_space( · line 1522
Quoted 4 lines
fs/xfs/xfs_bmap_util.c +0
-1@@ -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.
fs/xfs/xfs_bmap_util.c xfs_collapse_file_space( · line 1531
Quoted 9 lines
fs/xfs/xfs_bmap_util.c +1
-1/* * 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 reservesXFS_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.Quoted 7 lines
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_RESERVEis 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 completionSo here there is no need for it at all....
Quoted 7 lines
Quoted 4 lines
Yeah, dropping
XFS_TRANS_RESERVEmakes 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!
Quoted 7 lines
Cheers,
Dave.
--
Dave Chinner
[email protected]
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs