xfs
[Top] [All Lists]

Re: [PATCH 1/2] xfsprogs: ignore stripe geom if sunit or swidth == physi

To: Eric Sandeen <sandeen@xxxxxxxxxx>
Subject: Re: [PATCH 1/2] xfsprogs: ignore stripe geom if sunit or swidth == physical sector size
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Wed, 29 Oct 2014 14:37:22 -0400
Cc: xfs-oss <xfs@xxxxxxxxxxx>, Stan Hoeppner <stan@xxxxxxxxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <544FD3E1.1060000@xxxxxxxxxx>
References: <544FD3E1.1060000@xxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
On Tue, Oct 28, 2014 at 12:35:29PM -0500, Eric Sandeen wrote:
> Today, this geometry:
> 
> # modprobe scsi_debug  opt_blks=2048 dev_size_mb=2048
> # blockdev --getpbsz --getss --getiomin --getioopt  /dev/sdd
> 512
> 512
> 512
> 1048576
> 
> will result in a warning at mkfs time, like this:
> 
> # mkfs.xfs -f -d su=64k,sw=12 -l su=64k /dev/sdd
> mkfs.xfs: Specified data stripe width 1536 is not the same as the volume 
> stripe width 2048
> 
> because our geometry discovery thinks it looks like a
> valid striping setup which the commandline is overriding. 
> However, a stripe unit of 512 really isn't indicative of
> a proper stripe geometry.
> 

So the assumption is that the storage reports a non-physical block size
for minimum and optimal I/O sizes for geometry detection. There was a
real world scenario of this, right? Any idea of the configuration
details (e.g., raid layout) that resulted in an increased optimal I/O
size but not minimum I/O size?

This seems reasonable to me and the code looks fine, save a trailing
whitespace instance below. I'm just curious if there are any weird
corner cases where there's value in the reported optimal I/O size or the
real world situation was just noise.

> Prior to this patch, we reset only sunit *or* swidth,
> if either was equal to physical block size, but not
> necessarily both.
> 
> Change the heuristic so that if either the discovered
> sunit or the discovered swidth is physical block size,
> we reset *both* to zero and ignore the geom completely.
> 
> While we're at it, don't pass &dummy in for multiple 
> arguments to blkid_get_topology(); that'll mean that
> inside the function, the last assignment wins, and could
> lead to unexpected results.
> 
> Reported-by: Stan Hoeppner <stan@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 4546e35..a0fed31 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -410,21 +410,27 @@ static void blkid_get_topology(
>       *lsectorsize = val;
>       val = blkid_topology_get_physical_sector_size(tp);
>       *psectorsize = val;
> +     val = blkid_topology_get_minimum_io_size(tp);
> +     *sunit = val;
> +     val = blkid_topology_get_optimal_io_size(tp);
> +     *swidth = val;
>  
>       /*
> -      * Blkid reports the information in terms of bytes, but we want it in
> -      * terms of 512 bytes blocks (just to convert it to bytes later..)
> -      *
>        * If the reported values are the same as the physical sector size
> -      * do not bother to report anything.  It will just cause warnings
> +      * do not bother to report anything.  It will only cause warnings
>        * if people specify larger stripe units or widths manually.
>        */
> -     val = blkid_topology_get_minimum_io_size(tp);
> -     if (val > *psectorsize)
> -             *sunit = val >> 9;
> -     val = blkid_topology_get_optimal_io_size(tp);
> -     if (val > *psectorsize)
> -             *swidth = val >> 9;
> +     if (*sunit == *psectorsize || *swidth == *psectorsize) {
> +             *sunit = 0;
> +             *swidth = 0;
> +     }
> +
> +     /*
> +      * Blkid reports the information in terms of bytes, but we want it in
> +      * terms of 512 bytes blocks (only to convert it to bytes later..)
> +      */
> +     *sunit = *sunit >> 9;
> +     *swidth = *swidth >> 9; 

Trailing whitespace here.

Brian

>  
>       if (blkid_topology_get_alignment_offset(tp) != 0) {
>               fprintf(stderr,
> @@ -484,10 +490,10 @@ static void get_topology(
>       }
>  
>       if (xi->rtname && !xi->risfile) {
> -             int dummy;
> +             int sunit, lsectorsize, psectorsize;
>  
> -             blkid_get_topology(xi->rtname, &dummy, &ft->rtswidth,
> -                                &dummy, &dummy, force_overwrite);
> +             blkid_get_topology(xi->rtname, &sunit, &ft->rtswidth,
> +                                &lsectorsize, &psectorsize, force_overwrite);
>       }
>  }
>  #else /* ENABLE_BLKID */
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

<Prev in Thread] Current Thread [Next in Thread>