xfs
[Top] [All Lists]

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

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH V2] mkfs: handle 4k sector devices more cleanly
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Fri, 8 Jan 2010 12:44:00 -0500
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>, Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
In-reply-to: <4B476171.4020701@xxxxxxxxxxx>
References: <4B1E9A25.50108@xxxxxxxxxx> <4B476171.4020701@xxxxxxxxxxx>
User-agent: Mutt/1.5.19 (2009-01-05)
On Fri, Jan 08, 2010 at 10:46:41AM -0600, Eric Sandeen wrote:
> +extern void platform_findsizes (char *path, int fd, long long *sz, int *bsz);

Can you move the prototype from libxfs/init.h to include/libxfs.h
instead of adding it to the .c file?

> +     /*
> +      * 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;

This still confuses the heck out of me. What do you think about the
incremental patch at the end of the mail?

>       if (slflag || ssflag)
>               xi.setblksize = sectorsize;
> -     else
> -             xi.setblksize = 1;

So for the defaul case we now never set the sector size in the libxfs
init.  This seems safe to me, but why did we do it before?  Could
a previous user have left it set to a wrong value?

Maye we should just do the xi.setblksize = sectorsize unconditionally?


Index: xfsprogs-dev/mkfs/xfs_mkfs.c
===================================================================
--- xfsprogs-dev.orig/mkfs/xfs_mkfs.c   2010-01-08 18:33:53.619277529 +0100
+++ xfsprogs-dev/mkfs/xfs_mkfs.c        2010-01-08 18:39:37.758005711 +0100
@@ -1561,21 +1561,32 @@ main(
        memset(&ft, 0, sizeof(ft));
        get_topology(&xi, &ft);
 
-       /*
-        * MD wants sector size set == block size to avoid switching.
-        * Otherwise, if not specfied via command, use device sectorsize
-        */
+       if (ft.sectoralign) {
+               /*
+                * Older Linux software RAID versions want the sector size
+                * to match the block size to avoid switching I/O sizes.
+                * For the legacy libdisk case we thus set the sector size to
+                * match the block size.  For systems using libblkid we assume
+                * that the kernel is recent enough to not require this and
+                * ft.sectoralign will never be set.
+                */
+               sectorsize = blocksize;
+       } else if (!ssflag) {
+               /*
+                * Unless specified manually on the command line use the
+                * advertised sector size of the device.
+                */
+               sectorsize = ft.sectorsize;
+       }
+
        if (ft.sectoralign || !ssflag) {
-               if (ft.sectoralign)
-                       sectorsize = blocksize;
-               else
-                       sectorsize = ft.sectorsize;
                sectorlog = libxfs_highbit32(sectorsize);
                if (loginternal) {
                        lsectorsize = sectorsize;
                        lsectorlog = sectorlog;
                }
        }
+
        if (sectorsize < XFS_MIN_SECTORSIZE ||
            sectorsize > XFS_MAX_SECTORSIZE || sectorsize > blocksize) {
                fprintf(stderr, _("illegal sector size %d\n"), sectorsize);

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