| Message ID | 1482436822-31546-2-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Dec 22, 2016 at 09:00:18PM +0100, Christoph Hellwig wrote: > Setting aside 4 blocks globally for bmbt splits isn't all that useful, > as different threads can allocate space in parallel. Bump it to 4 > blocks per AG to allow each thread that is currently doing an > allocation to dip into it. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- Presumably this patch addresses the potential deadlock issues from the previous version, but the commit log description makes no mention of it whatsoever. While the code seems fine, I think the commit log description needs more information wrt to that situation and the relationship/dependency with minleft. > fs/xfs/libxfs/xfs_alloc.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c > index 5050056..0a46f84 100644 > --- a/fs/xfs/libxfs/xfs_alloc.c > +++ b/fs/xfs/libxfs/xfs_alloc.c > @@ -95,10 +95,7 @@ unsigned int > xfs_alloc_set_aside( > struct xfs_mount *mp) > { > - unsigned int blocks; > - > - blocks = 4 + (mp->m_sb.sb_agcount * XFS_ALLOC_AGFL_RESERVE); > - return blocks; > + return mp->m_sb.sb_agcount * (XFS_ALLOC_AGFL_RESERVE + 4); The comment above xfs_alloc_set_aside() already touches on the writeback situation, but why 4 blocks per ag? Wasn't the intent to use worst_indlen() since that's the base for minleft? Also, it looks like this causes a regression in xfs/004. On a quick look, we might just need a test update however... Brian > } > > /* > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" 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-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 04, 2017 at 09:33:51AM -0500, Brian Foster wrote: > Presumably this patch addresses the potential deadlock issues from the > previous version, but the commit log description makes no mention of it > whatsoever. While the code seems fine, I think the commit log > description needs more information wrt to that situation and the > relationship/dependency with minleft. Ok. > The comment above xfs_alloc_set_aside() already touches on the writeback > situation, but why 4 blocks per ag? Wasn't the intent to use > worst_indlen() since that's the base for minleft? No, I've given up on that. worst_indlen deals with the fact that for converting a delayed extent of a given length we might need multiple real extents, possible in different AGs. This version of the series keeps the previous minleft that is for just allocating a single extent in the AG - the callers will handle "short" returns from xfs_bmapi_write and just start a new allocation. And except for a corner case in the large directory block allocation code these are in a new / rolled over transaction. Fixing the latter also is on my todo list, but it's another big issue that so far hasn't trigger in practive, so I'd like to keep it in a separate series. > Also, it looks like this causes a regression in xfs/004. On a quick > look, we might just need a test update however... Yes. Hard to do in a series for the kernel, though :) -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 08, 2017 at 11:30:28AM +0100, Christoph Hellwig wrote: > On Wed, Jan 04, 2017 at 09:33:51AM -0500, Brian Foster wrote: > > Presumably this patch addresses the potential deadlock issues from the > > previous version, but the commit log description makes no mention of it > > whatsoever. While the code seems fine, I think the commit log > > description needs more information wrt to that situation and the > > relationship/dependency with minleft. > > Ok. > > > The comment above xfs_alloc_set_aside() already touches on the writeback > > situation, but why 4 blocks per ag? Wasn't the intent to use > > worst_indlen() since that's the base for minleft? > > No, I've given up on that. worst_indlen deals with the fact that > for converting a delayed extent of a given length we might need multiple > real extents, possible in different AGs. > > This version of the series keeps the previous minleft that is for just > allocating a single extent in the AG - the callers will handle "short" > returns from xfs_bmapi_write and just start a new allocation. And > except for a corner case in the large directory block allocation code > these are in a new / rolled over transaction. Fixing the latter also > is on my todo list, but it's another big issue that so far hasn't > trigger in practive, so I'd like to keep it in a separate series. > Ok, anything you can include in the commit log and/or comment that helps clarify that is appreciated. Brian > > Also, it looks like this causes a regression in xfs/004. On a quick > > look, we might just need a test update however... > > Yes. Hard to do in a series for the kernel, though :) > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" 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-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c index 5050056..0a46f84 100644 --- a/fs/xfs/libxfs/xfs_alloc.c +++ b/fs/xfs/libxfs/xfs_alloc.c @@ -95,10 +95,7 @@ unsigned int xfs_alloc_set_aside( struct xfs_mount *mp) { - unsigned int blocks; - - blocks = 4 + (mp->m_sb.sb_agcount * XFS_ALLOC_AGFL_RESERVE); - return blocks; + return mp->m_sb.sb_agcount * (XFS_ALLOC_AGFL_RESERVE + 4); } /*
Setting aside 4 blocks globally for bmbt splits isn't all that useful, as different threads can allocate space in parallel. Bump it to 4 blocks per AG to allow each thread that is currently doing an allocation to dip into it. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_alloc.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)