diff mbox

[4/4] xfs: don't rely on ->total in xfs_alloc_space_available

Message ID 1481644767-9098-5-git-send-email-hch@lst.de (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Christoph Hellwig Dec. 13, 2016, 3:59 p.m. UTC 
->total is a bit of an odd parameter passed down to the low-level
allocator all the way from the high-level callers.  It's supposed to
contain the maximum number of blocks to be allocated for the whole
transaction [1].

But in xfs_iomap_write_allocate we only convert existing delayed
allocations and thus only have a minimal block reservation for the
current transaction, so xfs_alloc_space_available can't use it for
the allocation decisions.  Use the maximum of args->total and the
calculated block requirement to make a decision.  We probably should
get rid of args->total eventually and instead apply ->minleft more
broadly, but that will require some extensive changes all over.

[1] which creates lots of confusion as most callers don't decrement it
once doing a first allocation.  But that's for a separate series.
---
 fs/xfs/libxfs/xfs_alloc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Christoph Hellwig Dec. 14, 2016, 7:38 p.m. UTC | #1 
On Wed, Dec 14, 2016 at 01:30:18PM -0500, Brian Foster wrote:
> >  	/* do we have enough contiguous free space for the allocation? */
> > +	alloc_len = args->minlen + (args->alignment - 1)+ args->minalignslop;
> 
> Whitespace nit, missing space:				^
> 
> Also, is the addition of braces intentional? I believe it is possible
> for args->alignment == 0.

It's mostly for explaining the - 1 to the reader as it took me a while
to figure out what it was for.
--
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
Brian Foster Dec. 14, 2016, 9:51 p.m. UTC | #2 
On Wed, Dec 14, 2016 at 08:38:02PM +0100, Christoph Hellwig wrote:
> On Wed, Dec 14, 2016 at 01:30:18PM -0500, Brian Foster wrote:
> > >  	/* do we have enough contiguous free space for the allocation? */
> > > +	alloc_len = args->minlen + (args->alignment - 1)+ args->minalignslop;
> > 
> > Whitespace nit, missing space:				^
> > 
> > Also, is the addition of braces intentional? I believe it is possible
> > for args->alignment == 0.
> 
> It's mostly for explaining the - 1 to the reader as it took me a while
> to figure out what it was for.

Oh, care to elaborate? :)

Brian
--
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
Christoph Hellwig Dec. 15, 2016, 8:55 a.m. UTC | #3 
On Wed, Dec 14, 2016 at 04:51:58PM -0500, Brian Foster wrote:
> On Wed, Dec 14, 2016 at 08:38:02PM +0100, Christoph Hellwig wrote:
> > On Wed, Dec 14, 2016 at 01:30:18PM -0500, Brian Foster wrote:
> > > >  	/* do we have enough contiguous free space for the allocation? */
> > > > +	alloc_len = args->minlen + (args->alignment - 1)+ args->minalignslop;
> > > 
> > > Whitespace nit, missing space:				^
> > > 
> > > Also, is the addition of braces intentional? I believe it is possible
> > > for args->alignment == 0.
> > 
> > It's mostly for explaining the - 1 to the reader as it took me a while
> > to figure out what it was for.
> 
> Oh, care to elaborate? :)

the alignment is the boundary we want to align to, so we'll have to add
alignment - 1 at max to achive it. 
--
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
Brian Foster Dec. 15, 2016, noon UTC | #4 
On Thu, Dec 15, 2016 at 09:55:34AM +0100, Christoph Hellwig wrote:
> On Wed, Dec 14, 2016 at 04:51:58PM -0500, Brian Foster wrote:
> > On Wed, Dec 14, 2016 at 08:38:02PM +0100, Christoph Hellwig wrote:
> > > On Wed, Dec 14, 2016 at 01:30:18PM -0500, Brian Foster wrote:
> > > > >  	/* do we have enough contiguous free space for the allocation? */
> > > > > +	alloc_len = args->minlen + (args->alignment - 1)+ args->minalignslop;
> > > > 
> > > > Whitespace nit, missing space:				^
> > > > 
> > > > Also, is the addition of braces intentional? I believe it is possible
> > > > for args->alignment == 0.
> > > 
> > > It's mostly for explaining the - 1 to the reader as it took me a while
> > > to figure out what it was for.
> > 
> > Oh, care to elaborate? :)
> 
> the alignment is the boundary we want to align to, so we'll have to add
> alignment - 1 at max to achive it. 

Gotcha, so perhaps we should check that alignment != 0?

Brian

> --
> 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 mbox

Patch

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 87328a8..85039e5 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -1998,7 +1998,7 @@  xfs_alloc_space_available(
 	int			flags)
 {
 	struct xfs_perag	*pag = args->pag;
-	xfs_extlen_t		longest;
+	xfs_extlen_t		alloc_len, longest;
 	xfs_extlen_t		reservation; /* blocks that are still reserved */
 	int			available;
 
@@ -2008,15 +2008,16 @@  xfs_alloc_space_available(
 	reservation = xfs_ag_resv_needed(pag, args->resv);
 
 	/* do we have enough contiguous free space for the allocation? */
+	alloc_len = args->minlen + (args->alignment - 1)+ args->minalignslop;
 	longest = xfs_alloc_longest_free_extent(args->mp, pag, min_free,
 			reservation);
-	if ((args->minlen + args->alignment + args->minalignslop - 1) > longest)
+	if (longest < alloc_len)
 		return false;
 
 	/* do we have enough free space remaining for the allocation? */
 	available = (int)(pag->pagf_freeblks + pag->pagf_flcount -
 			  reservation - min_free - args->minleft);
-	if (available < (int)args->total)
+	if (available < (int)max(args->total, alloc_len))
 		return false;
 
 	/*