xfs
[Top] [All Lists]

Re: [PATCH] mkfs.xfs: always use underlying fs sector size when mkfs'ing

To: Jan Tulak <jtulak@xxxxxxxxxx>, Eric Sandeen <sandeen@xxxxxxxxxx>
Subject: Re: [PATCH] mkfs.xfs: always use underlying fs sector size when mkfs'ing a file
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Thu, 18 Jun 2015 09:57:09 -0500
Cc: xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <14667996.16447259.1434625419793.JavaMail.zimbra@xxxxxxxxxx>
References: <55820229.1010701@xxxxxxxxxx> <14667996.16447259.1434625419793.JavaMail.zimbra@xxxxxxxxxx>
On 6/18/15 6:03 AM, Jan Tulak wrote:
> 
> 
> ----- Original Message -----
>> From: "Eric Sandeen" <sandeen@xxxxxxxxxx>
>> To: "xfs-oss" <xfs@xxxxxxxxxxx>
>> Sent: Thursday, June 18, 2015 1:26:33 AM
>> Subject: [PATCH] mkfs.xfs: always use underlying fs sector size when 
>> mkfs'ing        a file
>>
>> If we are mkfs'ing a file, and that file is on a 4k sector filesystem,
>> we should make the fs image file with the same sector size, or things
>> may fail when they try to do direct IO in 512 byte chunks (depending
>> on whether it is a 512e or "hard" 4k device).
>>
>> Earlier commits attempted this to some degree:
>>
>> 5a7d59 xfsprogs: try to handle mkfs of a file on 4k sector device
>> 3800a2 mkfs.xfs: don't call blkid_get_topology on existing regular files
>>
>> but inexplicably missed the case where mkfs.xfs with "-d file" was
>> specified.
>>
>> One more try; in get_topology(), try to get the underlying fs sector
>> size in *all* cases where we are mkfs'ing a file, and set the sector size
>> accordingly.
>>
>> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
>> ---
>>
>> (This does it for 512e as well as hard 4k drives, but I think that's
>> probably ok?  If not, perhaps we should go further and attempt to
>> discern logical and physical sectors for the device under the
>> filesystem.  Is it worth it?  Not sure it is.)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index e2a052d..e44c390 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -462,31 +462,34 @@ static void get_topology(
>>      struct fs_topology      *ft,
>>      int                     force_overwrite)
>>  {
>> -    if (!xi->disfile) {
>> -            char *dfile = xi->volname ? xi->volname : xi->dname;
>> -            struct stat statbuf;
>> +    struct stat statbuf;
>> +    char *dfile = xi->volname ? xi->volname : xi->dname;
>>  
>> -            /*
>> -             * If our target is a regular file, and xi->disfile isn't
>> -             * set (i.e. no "-d file" invocation), use platform_findsizes
>> -             * to try to obtain the underlying filesystem's requirements
>> -             * for direct IO; we'll set our sector size to that if possible.
>> -             */
>> -            if (!stat(dfile, &statbuf) && S_ISREG(statbuf.st_mode)) {
>> -                    int fd;
>> -                    long long dummy;
>> -
>> -                    fd = open(dfile, O_RDONLY);
>> -                    if (fd >= 0) {
>> -                            platform_findsizes(dfile, fd, &dummy,
>> -                                               &ft->lsectorsize);
>> -                            close(fd);
>> -                    }
>> -            } else {
>> -                    blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth,
>> -                                       &ft->lsectorsize, &ft->psectorsize,
>> -                                       force_overwrite);
>> +    /*
>> +     * If our target is a regular file, use platform_findsizes
>> +     * to try to obtain the underlying filesystem's requirements
>> +     * for direct IO; we'll set our sector size to that if possible.
>> +     */
>> +    if (xi->disfile ||
>> +        (!stat(dfile, &statbuf) && S_ISREG(statbuf.st_mode))) {
>> +            int fd;
>> +            int flags = O_RDONLY;
>> +            long long dummy;
>> +
>> +            /* with xi->disfile we may not have the file yet! */
>> +            if (xi->disfile)
>> +                    flags |= O_CREAT;
>> +
>> +            fd = open(dfile, flags, 0666);
>> +            if (fd >= 0) {
>> +                    platform_findsizes(dfile, fd, &dummy, &ft->lsectorsize);
>> +                    close (fd);
>>              }
>> +
>> +    } else {
>> +            blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth,
>> +                               &ft->lsectorsize, &ft->psectorsize,
>> +                               force_overwrite);
>>      }
>>  
>>      if (xi->rtname && !xi->risfile) {
>>
> 
> This changes get_topology only for ENABLE_BLKID branch of #ifdef. Is
> that intentional, i.e. we don't expect anyone not using ENABLE_BLKID?
> Because otherwise, if mkfs is compiled without ENABLE_BLKID, then all
> we get is:

Hm, yeah, good point.  I always forget about this.  :(  I can send V2.

And sorry if this overlaps w/ your changes- I got a bug report about
xfstests failing when testing hard 4k devices, and it was due to image
files created on a filesystem on that hard 4k device, and xfsprogs tools
fail when they try to do 512-byte direct IO to the image.

Thanks,
-Eric

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