diff mbox

[2/5] xfs: fix the alignment fallback in xfs_bmap_btalloc

Message ID 1482436822-31546-3-git-send-email-hch@lst.de (mailing list archive)
State Accepted
Headers show

Commit Message

Christoph Hellwig Dec. 22, 2016, 8 p.m. UTC
The way the alignment field is used the minimum possible alignment is 1.
By setting it to 0 we'll mess up calculations that rely on this.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Brian Foster Jan. 4, 2017, 2:34 p.m. UTC | #1
On Thu, Dec 22, 2016 at 09:00:19PM +0100, Christoph Hellwig wrote:
> The way the alignment field is used the minimum possible alignment is 1.
> By setting it to 0 we'll mess up calculations that rely on this.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 2760bc3..19c05e9 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3796,7 +3796,7 @@ xfs_bmap_btalloc(
>  		 */
>  		args.type = atype;
>  		args.fsbno = ap->blkno;
> -		args.alignment = 0;
> +		args.alignment = 1;

Ok, but we have the following near the top of xfs_alloc_vextent():

        if (args->alignment == 0)
                args->alignment = 1;

... so I'm not sure the commit log description is accurate. That aside:

Reviewed-by: Brian Foster <bfoster@redhat.com>

Brian

>  		if ((error = xfs_alloc_vextent(&args)))
>  			return error;
>  	}
> -- 
> 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
Christoph Hellwig Jan. 8, 2017, 10:31 a.m. UTC | #2
On Wed, Jan 04, 2017 at 09:34:05AM -0500, Brian Foster wrote:
> Ok, but we have the following near the top of xfs_alloc_vextent():
> 
>         if (args->alignment == 0)
>                 args->alignment = 1;
> 
> ... so I'm not sure the commit log description is accurate. That aside:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Well, in that case we might not actually need it.  This was just a
a drive-by patch from investigating what the alignmnet - 1 mean in
the allocator.

Either way this could be cleaned up, but probably shouldn't be in
a minimal bugfix series.
--
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 Jan. 8, 2017, 4:08 p.m. UTC | #3
On Sun, Jan 08, 2017 at 11:31:34AM +0100, Christoph Hellwig wrote:
> On Wed, Jan 04, 2017 at 09:34:05AM -0500, Brian Foster wrote:
> > Ok, but we have the following near the top of xfs_alloc_vextent():
> > 
> >         if (args->alignment == 0)
> >                 args->alignment = 1;
> > 
> > ... so I'm not sure the commit log description is accurate. That aside:
> > 
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> Well, in that case we might not actually need it.  This was just a
> a drive-by patch from investigating what the alignmnet - 1 mean in
> the allocator.
> 
> Either way this could be cleaned up, but probably shouldn't be in
> a minimal bugfix series.

We could also just replace it with something like ASSERT(args->alignment
> 0) before the use of (alignment - 1), which is kind of
self-documenting (but still probably not worth a patch on its own). But
fair enough either way...

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_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 2760bc3..19c05e9 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3796,7 +3796,7 @@  xfs_bmap_btalloc(
 		 */
 		args.type = atype;
 		args.fsbno = ap->blkno;
-		args.alignment = 0;
+		args.alignment = 1;
 		if ((error = xfs_alloc_vextent(&args)))
 			return error;
 	}