xfs
[Top] [All Lists]

Re: [PATCH] mkfs: handle 4k sector devices more cleanly

To: Eric Sandeen <sandeen@xxxxxxxxxx>
Subject: Re: [PATCH] mkfs: handle 4k sector devices more cleanly
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 10 Dec 2009 17:40:37 -0500
Cc: xfs-oss <xfs@xxxxxxxxxxx>
In-reply-to: <4B1E9A25.50108@xxxxxxxxxx>
References: <4B1E9A25.50108@xxxxxxxxxx>
User-agent: Mutt/1.5.19 (2009-01-05)
On Tue, Dec 08, 2009 at 12:25:41PM -0600, Eric Sandeen wrote:
> Trying to mkfs a 4k sector device today fails w/o manually specifying
> sector size:
> 
> # modprobe scsi_debug sector_size=4096 dev_size_mb=32
> # mkfs.xfs -f /dev/sdc
> mkfs.xfs: warning - cannot set blocksize on block device /dev/sdc: Invalid 
> argument
> Warning: the data subvolume sector size 512 is less than the sector size 
> reported by the device (4096).
> ... <fail>
> 
> add sectorsize to the device topology info, and use that if present.
> 
> Also check that explicitly requested sector sizes are not smaller
> than the hardware size.  This already fails today, but with the more
> cryptic "cannot set blocksize" ioctl error above.
> 
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
> 
> diff --git a/libxfs/linux.c b/libxfs/linux.c
> index bc49903..2e07d54 100644
> --- a/libxfs/linux.c
> +++ b/libxfs/linux.c
> @@ -112,9 +112,9 @@ platform_set_blocksize(int fd, char *path, dev_t device, 
> int blocksize, int fata
>       if (major(device) != RAMDISK_MAJOR) {
>               if ((error = ioctl(fd, BLKBSZSET, &blocksize)) < 0) {
>                       fprintf(stderr, _("%s: %s - cannot set blocksize "
> -                                     "on block device %s: %s\n"),
> +                                     "%d on block device %s: %s\n"),
>                               progname, fatal ? "error": "warning",
> -                             path, strerror(errno));
> +                             blocksize, path, strerror(errno));

Defintively a more useful error message than before, thanks.

> -static void blkid_get_topology(const char *device, int *sunit, int *swidth)
> +static void blkid_get_topology(const char *device, int *sunit, int *swidth, 
> int *sectorsize)
>  {
>       blkid_topology tp;
>       blkid_probe pr;
> @@ -348,7 +349,9 @@ static void blkid_get_topology(const char *device, int 
> *sunit, int *swidth)
>       val = blkid_topology_get_optimal_io_size(tp) >> 9;
>       if (val > 1)
>               *swidth = val;
> -
> +     val = blkid_probe_get_sectorsize(pr);
> +     if (val > 1)
> +             *sectorsize = val;

I don't think the val > 1 check here makes any sense.

> +             blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth, 
> &ft->sectorsize);

The lines is growing a bit too long here..


Also I think you should add the sector size retrival by using the ioctl
directly for the non-blkid case to make sure the code doesn't have too
many corner cases.

> -     if (ft.sectoralign) {
> -             sectorsize = blocksize;
> +     /*
> +      * MD wants sector size set == block size to avoid switching.
> +      * Otherwise, if not specfied via command, use device sectorsize
> +      */
> +     if (ft.sectoralign || !ssflag) {
> +             if (ft.sectoralign)
> +                     sectorsize = blocksize;
> +             else
> +                     sectorsize = ft.sectorsize;

The code looks good, but I don't think the comment helps understanding
what's going on.  And I might get a bit pendantic here, but changing it
to

        if (!ssflag || ft.sectoralign)

might make the intent a bit more clear.


> +     if (sectorsize < ft.sectorsize) {
> +             fprintf(stderr, _("illegal sector size %d; hw sector is %d\n"),
> +                     sectorsize, ft.sectorsize);
> +             usage();
> +     }

Looks good.

>       if (lsectorsize < XFS_MIN_SECTORSIZE ||
>           lsectorsize > XFS_MAX_SECTORSIZE || lsectorsize > blocksize) {
>               fprintf(stderr, _("illegal log sector size %d\n"), lsectorsize);
> @@ -1749,10 +1764,10 @@ main(
>       calc_stripe_factors(dsu, dsw, sectorsize, lsu, lsectorsize,
>                               &dsunit, &dswidth, &lsunit);
>  
> -     if (slflag || ssflag)
> +     if (slflag || ssflag || ft.setorsize)

There's a c missing here, this wouldn't even compile :)

It will also be always true for blkid builds which is at least a bit
confusing.  If you also updated the libdisk case as suggested above we
could also get rid of the xi.setblksize = 1 special case totally and
always pass the proper block size to libxfs_init and libxfs_device_open
(and make libxfs_device_open static in libxfs/init.c while we're at it
:))

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