<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><span style="font-family:arial,sans-serif">On Fri, Apr 8, 2016 at 2:25 AM, Eric Sandeen </span><span dir="ltr" style="font-family:arial,sans-serif"><<a href="mailto:sandeen@sandeen.net" target="_blank">sandeen@sandeen.net</a>></span><span style="font-family:arial,sans-serif"> wrote:</span></div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">
On 3/24/16 6:15 AM, <a href="mailto:jtulak@redhat.com">jtulak@redhat.com</a> wrote:<br>
> From: Dave Chinner <<a href="mailto:dchinner@redhat.com">dchinner@redhat.com</a>><br>
><br>
> CHANGELOG<br>
> o Fix where xi.dname was incorrectly used instead of dfile<br>
> o Variable alignment (tabs)<br>
> o Added error handling for stat/statfs in init.c<br>
> o Remove a duplicate pread in zero_old_xfs_structures and for the<br>
> remaining call, save the return value in a more meaningful variable.<br>
> o A chunk moved to previous patch.<br>
><br>
> If the device is actually a file, and "-d file" is not specified,<br>
> mkfs will try to treat it as a block device and get stuff wrong.<br>
> Image files don't necessarily have the same sector sizes as the<br>
> block device or filesystem underlying the image file, nor should we<br>
> be issuing discard ioctls on image files.<br>
><br>
> To fix this sanely, only require "-d file" if the device name is<br>
> invalid to trigger creation of the file. Otherwise, use stat() to<br>
> determine if the device is a file or block device and deal with that<br>
> appropriately by setting the "isfile" variables and turning off<br>
> direct IO. Then ensure that we check the "isfile" options before<br>
> doing things that are specific to block devices.  Also, as direct IO<br>
> is disabled for files, use statfs() for getting host FS blocksize,<br>
> not platform_findsizes().<br>
><br>
> These changes, however, can cause some tests to fail when the test<br>
> partition on which the file is created has blocksize bigger than 512.<br>
> Before, the underlying fs was ignored. Now, an attempt to create<br>
> a fs in a file with blocksize 512 on a 4096 underlying partition will<br>
> fail.<br>
><br>
> Other file/blockdev issues fixed:<br>
>       - use getstr to detect specifying the data device name<br>
>         twice.<br>
>       - check file/size/name parameters before anything else.<br>
>       - overwrite checks need to be done before the image file is<br>
>         opened and potentially truncated.<br>
>       - blkid_get_topology() should not be called for image files,<br>
>         so warn when it is called that way.<br>
>       - zero_old_xfs_structures() emits a spurious error:<br>
>               "existing superblock read failed: Success"<br>
>         when it is run on a truncated image file. Don't warn if we<br>
>         see this problem on an image file.<br>
>       - Don't issue discards on image files.<br>
>       - Use fsync() for image files, not BLKFLSBUF in<br>
>         platform_flush_device() for Linux.<br>
<br>
</div></div>This one causes at least one interesting issue:<br>
<br>
#mkfs/mkfs.xfs<br>
Error accessing specified device (null): Bad address<br>
Usage: mkfs.xfs<br>
...<br>
<br>
because:<br>
<br>
        check_device_type(dfile, &xi.disfile, !dsize, !dfile,<br>
<span class="">                          Nflag ? NULL : &xi.dcreat, force_overwrite, "d");<br>
<br>
</span>so "dfile" can be NULL, but that function immediately tries to stat it.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​A simple if NULL, then usage() should take care of this...</div></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline"><br></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div class="h5"><br>
> Signed-off-by: Dave Chinner <<a href="mailto:dchinner@redhat.com">dchinner@redhat.com</a>><br>
> Signed-off-by: Jan Tulak <<a href="mailto:jtulak@redhat.com">jtulak@redhat.com</a>><br>
> ---<br>
>  libxfs/init.c   |  12 ++++<br>
>  libxfs/linux.c  |  12 +++-<br>
>  mkfs/xfs_mkfs.c | 181 ++++++++++++++++++++++++++++++++++++++------------------<br>
>  3 files changed, 147 insertions(+), 58 deletions(-)<br>
><br>
> diff --git a/libxfs/init.c b/libxfs/init.c<br>
> index 8d747e8..268136f 100644<br>
> --- a/libxfs/init.c<br>
> +++ b/libxfs/init.c<br>
> @@ -246,6 +246,9 @@ libxfs_init(libxfs_init_t *a)<br>
>       char            rtpath[25];<br>
>       int             rval = 0;<br>
>       int             flags;<br>
> +     struct          stat st;<br>
> +     struct          statfs stfs;<br>
> +     int             statres;<br>
><br>
>       dpath[0] = logpath[0] = rtpath[0] = '\0';<br>
>       dname = a->dname;<br>
> @@ -278,6 +281,15 @@ libxfs_init(libxfs_init_t *a)<br>
>                       a->ddev= libxfs_device_open(dname, a->dcreat, flags,<br>
>                                                   a->setblksize);<br>
>                       a->dfd = libxfs_device_to_fd(a->ddev);<br>
> +                     statres = stat(dname, &st);<br>
> +                     statres += statfs(dname, &stfs);<br>
> +                     if(statres){<br>
</div></div>                          ^space   ^space<br>
<span class="">> +                             fprintf(stderr, _("%s: stat failed.\n"),<br>
> +                                     progname);<br>
> +                             goto done;<br>
> +                     }<br>
> +                     a->dsize = st.st_size/BBSIZE;<br>
> +                     a->dbsize = stfs.f_bsize;<br>
<br>
</span>ok so for a file you choose ->dsize to be file size in 512-sector units,<br>
and ->dbsize to be the fs block size.<br>
<br>
This is all under if (a->disfile); if we didn't actually specify "-dfile"<br>
but it *is* a file, then we get to platform_findsizes() - which handles<br>
files.  And handles them differently.   </blockquote><div><br></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">​No, a->disfile is set to 1 implicitly if the target is a file in check_device_type():</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><div class="gmail_default">1070        if (S_ISREG(statbuf.st_mode)) {</div><div class="gmail_default">1071               if (!*isfile)</div><div class="gmail_default">1072                      *isfile = 1;</div><div class="gmail_default"><br></div></div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hm but you removed that (see below)<br>
and added more stat() calls...?<br>
<br>
What is the reason for adding these stats at this point?<br></blockquote><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​What is removed? Where exactly? Or it should be "above"?</div></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline"><span style="color:rgb(80,0,80);font-family:arial,sans-serif">> +                     statres = stat(dname, &st);</span><br style="color:rgb(80,0,80);font-family:arial,sans-serif"><span style="color:rgb(80,0,80);font-family:arial,sans-serif">> +                     statres += statfs(dname, &stfs);</span><br></div></div>This is to get dsize and dbsize values for the file.</div><div class="gmail_quote"><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
(and if there's a reason, why only for ->disfile but not ->lisfile?)<br></blockquote><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​Because I forgot or didn't noticed. :-) Adding to lisfile and rtfile too.</div></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​</div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div class="h5"><br>
>               } else {<br>
>                       if (!check_open(dname, flags, &rawfile, &blockfile))<br>
>                               goto done;<br>
> diff --git a/libxfs/linux.c b/libxfs/linux.c<br>
> index f6ea1b2..adb8ff1 100644<br>
> --- a/libxfs/linux.c<br>
> +++ b/libxfs/linux.c<br>
> @@ -18,6 +18,7 @@<br>
><br>
>  #define ustat __kernel_ustat<br>
>  #include <mntent.h><br>
> +#include <sys/vfs.h><br>
>  #include <sys/stat.h><br>
>  #undef ustat<br>
>  #include <sys/ustat.h><br>
> @@ -125,7 +126,16 @@ platform_set_blocksize(int fd, char *path, dev_t device, int blocksize, int fata<br>
>  void<br>
>  platform_flush_device(int fd, dev_t device)<br>
>  {<br>
> -     if (major(device) != RAMDISK_MAJOR)<br>
> +     struct stat64   st;<br>
> +     if (major(device) == RAMDISK_MAJOR)<br>
> +             return;<br>
> +<br>
> +     if (fstat64(fd, &st) < 0)<br>
> +             return;<br>
> +<br>
> +     if (S_ISREG(st.st_mode))<br>
> +             fsync(fd);<br>
> +     else<br>
>               ioctl(fd, BLKFLSBUF, 0);<br>
>  }<br>
><br>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c<br>
> index 9261ed5..7bd9fd5 100644<br>
> --- a/mkfs/xfs_mkfs.c<br>
> +++ b/mkfs/xfs_mkfs.c<br>
> @@ -787,7 +787,7 @@ calc_stripe_factors(<br>
>  #ifdef ENABLE_BLKID<br>
>  static int<br>
>  check_overwrite(<br>
> -     char            *device)<br>
> +     const char      *device)<br>
>  {<br>
>       const char      *type;<br>
>       blkid_probe     pr = NULL;<br>
> @@ -804,7 +804,7 @@ check_overwrite(<br>
>       fd = open(device, O_RDONLY);<br>
>       if (fd < 0)<br>
>               goto out;<br>
> -     platform_findsizes(device, fd, &size, &bsz);<br>
> +     platform_findsizes((char *)device, fd, &size, &bsz);<br>
>       close(fd);<br>
><br>
>       /* nothing to overwrite on a 0-length device */<br>
> @@ -851,7 +851,6 @@ check_overwrite(<br>
>                       "according to blkid\n"), progname, device);<br>
>       }<br>
>       ret = 1;<br>
> -<br>
>  out:<br>
>       if (pr)<br>
>               blkid_free_probe(pr);<br>
> @@ -877,8 +876,12 @@ static void blkid_get_topology(<br>
>       struct stat statbuf;<br>
><br>
>       /* can't get topology info from a file */<br>
> -     if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode))<br>
> +     if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode)) {<br>
> +             fprintf(stderr,<br>
> +     _("%s: Warning: trying to probe topology of a file %s!\n"),<br>
> +                     progname, device);<br>
>               return;<br>
> +     }<br>
><br>
>       pr = blkid_new_probe_from_filename(device);<br>
>       if (!pr)<br>
> @@ -976,35 +979,35 @@ static void get_topology(<br>
>       struct fs_topology      *ft,<br>
>       int                     force_overwrite)<br>
>  {<br>
> -     struct stat statbuf;<br>
>       char *dfile = xi->volname ? xi->volname : xi->dname;<br>
> +     struct stat statbuf;<br>
> +     struct statfs statfsbuf;<br>
><br>
>       /*<br>
> -      * If our target is a regular file, use platform_findsizes<br>
> -      * to try to obtain the underlying filesystem's requirements<br>
> -      * for direct IO; we'll set our sector size to that if possible.<br>
> +      * If our target is a regular file, use statfs<br>
> +      * to try to obtain the underlying filesystem's blocksize.<br>
>        */<br>
>       if (xi->disfile ||<br>
> -         (!stat(dfile, &statbuf) && S_ISREG(statbuf.st_mode))) {<br>
> +             (!stat(dfile, &statbuf) && S_ISREG(statbuf.st_mode))) {<br>
<br>
</div></div>dave pointed out that this indentation "fix" is incorrect, the line is fine as<br>
it is; it's part of the same conditional; it shouldn't be tabbed into the code<br>
block under the conditional.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​I'm removing this change.</div></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
>               int fd;<br>
>               int flags = O_RDONLY;<br>
> -             long long dummy;<br>
><br>
>               /* with xi->disfile we may not have the file yet! */<br>
>               if (xi->disfile)<br>
>                       flags |= O_CREAT;<br>
><br>
>               fd = open(dfile, flags, 0666);<br>
> +<br>
>               if (fd >= 0) {<br>
> -                     platform_findsizes(dfile, fd, &dummy, &ft->lsectorsize);<br>
> +                     fstatfs(fd, &statfsbuf);<br>
<br>
</span>no error checking on fstatfs, but...<br></blockquote><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​Added.​</div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
> +                     ft->lsectorsize = statfsbuf.f_bsize;<br>
<br>
</span>Ok, platform_findsizes already explicitly handled regular files, and tries to<br>
find out via an xfs ioctl what the minimum DIO size is, and uses that for<br>
the sector size for the filesystem in the iamge. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Now you stat & get the blocksize, and use that instead, but it's likely<br>
to be different:<br>
<br>
i.e. before:<br>
<br>
# mkfs/mkfs.xfs -f fsfile<br>
meta-data=fsfile                 isize=512    agcount=4, agsize=65536 blks<br>
         =                       sectsz=512   attr=2, projid32bit=1<br>
<br>
after:<br>
<br>
# mkfs/mkfs.xfs -f fsfile<br>
meta-data=fsfile                 isize=512    agcount=4, agsize=65536 blks<br>
         =                       sectsz=4096  attr=2, projid32bit=1<br>
<br>
and also, now:<br>
<br>
# mkfs/mkfs.xfs -f -dfile,name=fsfile,size=1g -b size=2048<br>
block size 2048 cannot be smaller than logical sector size 4096<br>
<br>
What prompted you to make this change, was there some other problem you<br>
needed to fix?<br></blockquote><div><br></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">​But DIO is disabled for the files, per the commit message:</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">[...] and turning off</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">direct IO. Then ensure that we check the "isfile" options before</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">doing things that are specific to block devices.  Also, as direct IO</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">is disabled for files, use statfs() for getting host FS blocksize,</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">not platform_findsizes().​</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">So we have to use whatever the underlying fs tells us, not what the physical device has, right?</div><br></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">​Rather, I wonder if there is any reason to keep the platform_findsizes part about regular files - it shouldn't get into the branch ever.</div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
>                       close(fd);<br>
> -                     ft->psectorsize = ft->lsectorsize;<br>
<br>
</span>hm, now psectorsize isn't set at all?<br></blockquote><div><br></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​This looks like a bug, I think the assignment should stay here.</div></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​</div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
>               } else<br>
>                       ft->psectorsize = ft->lsectorsize = BBSIZE;<br>
>       } else {<br>
>               blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth,<br>
> -                                &ft->lsectorsize, &ft->psectorsize,<br>
> -                                force_overwrite);<br>
> +                                     &ft->lsectorsize, &ft->psectorsize,<br>
> +                                     force_overwrite);<br>
<br>
</span>please don't change these lines, they line up w/ the function opening paren as<br>
they should.<br>
<div><div class="h5"><br></div></div></blockquote><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">​Sure. ​</div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="h5">
>       }<br>
><br>
>       if (xi->rtname && !xi->risfile) {<br>
> @@ -1016,6 +1019,75 @@ static void get_topology(<br>
>  }<br>
><br>
>  static void<br>
> +check_device_type(<br>
> +     const char      *name,<br>
> +     int             *isfile,<br>
> +     bool            no_size,<br>
> +     bool            no_name,<br>
> +     int             *create,<br>
> +     bool            force_overwrite,<br>
> +     const char      *optname)<br>
> +{<br>
> +     struct stat64 statbuf;<br>
> +<br>
> +     if (*isfile && (no_size || no_name)) {<br>
> +             fprintf(stderr,<br>
> +     _("if -%s file then -%s name and -%s size are required\n"),<br>
> +                     optname, optname, optname);<br>
> +             usage();<br>
> +     }<br>
> +<br>
> +     if (stat64(name, &statbuf)) {<br>
> +             if (errno == ENOENT && *isfile) {<br>
> +                     if (create)<br>
> +                             *create = 1;<br>
> +                     return;<br>
> +             }<br>
> +<br>
> +             fprintf(stderr,<br>
> +     _("Error accessing specified device %s: %s\n"),<br>
> +                             name, strerror(errno));<br>
> +             usage();<br>
> +             return;<br>
> +     }<br>
> +<br>
> +     if (!force_overwrite && check_overwrite(name)) {<br>
> +             fprintf(stderr,<br>
> +     _("%s: Use the -f option to force overwrite.\n"),<br>
> +                     progname);<br>
> +             exit(1);<br>
> +     }<br>
> +<br>
> +     /*<br>
> +      * We only want to completely truncate and recreate an existing file if<br>
> +      * we were specifically told it was a file. Set the create flag only in<br>
> +      * this case to trigger that behaviour.<br>
> +      */<br>
> +     if (S_ISREG(statbuf.st_mode)) {<br>
> +             if (!*isfile)<br>
> +                     *isfile = 1;<br>
> +             else if (create)<br>
> +                     *create = 1;<br>
> +             return;<br>
> +     }<br>
> +<br>
> +     if (S_ISBLK(statbuf.st_mode)) {<br>
> +             if (*isfile) {<br>
> +                     fprintf(stderr,<br>
> +     _("specified \"-%s file\" on a block device %s\n"),<br>
> +                             optname, name);<br>
> +                     usage();<br>
> +             }<br>
> +             return;<br>
> +     }<br>
> +<br>
> +     fprintf(stderr,<br>
> +     _("specified device %s not a file or block device\n"),<br>
> +             name);<br>
> +     usage();<br>
> +}<br>
> +<br>
> +static void<br>
>  fixup_log_stripe_unit(<br>
>       int             lsflag,<br>
>       int             sunit,<br>
> @@ -1279,7 +1351,6 @@ zero_old_xfs_structures(<br>
>       __uint32_t              bsize;<br>
>       int                     i;<br>
>       xfs_off_t               off;<br>
> -     int                     tmp;<br>
><br>
>       /*<br>
>        * We open regular files with O_TRUNC|O_CREAT. Nothing to do here...<br>
> @@ -1299,15 +1370,23 @@ zero_old_xfs_structures(<br>
>       }<br>
>       memset(buf, 0, new_sb->sb_sectsize);<br>
><br>
> -     tmp = pread(xi->dfd, buf, new_sb->sb_sectsize, 0);<br>
> -     if (tmp < 0) {<br>
> +     off = pread(xi->dfd, buf, new_sb->sb_sectsize, 0);<br>
> +     if (off < 0) {<br>
>               fprintf(stderr, _("existing superblock read failed: %s\n"),<br>
>                       strerror(errno));<br>
>               goto done;<br>
>       }<br>
> -     if (tmp != new_sb->sb_sectsize) {<br>
> -             fprintf(stderr,<br>
> -     _("warning: could not read existing superblock, skip zeroing\n"));<br>
> +     /*<br>
> +      * If we are creating an image file, it might be of zero length at this<br>
> +      * point in time. Hence reading the existing superblock is going to<br>
> +      * return zero bytes. It's not a failure we need to warn about in this<br>
> +      * case.<br>
> +      */<br><br></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="h5">
</div></div>except you already did "if (off < 0) fail" above this.<br>
<br>
Ok, at this point I think it might be best to revert to Dave's original version.<br>
<br>
If there were specific problems you were trying to address, can you point them out?<br>
<br>
Thanks,<br>
-Eric<br>
<div class=""><div class="h5"><span style="color:rgb(34,34,34)"><br></span></div></div></blockquote><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​(inserting your next email)​</div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5"><span style="color:rgb(34,34,34)"><br>On 4/7/16 7:25 PM, Eric Sandeen wrote:<br>>> @@ -1299,15 +1370,23 @@ zero_old_xfs_structures(<br>>> >    }<br>>> >    memset(buf, 0, new_sb->sb_sectsize);<br>>> ><br>>> > -  tmp = pread(xi->dfd, buf, new_sb->sb_sectsize, 0);<br>>> > -  if (tmp < 0) {<br>>> > +  off = pread(xi->dfd, buf, new_sb->sb_sectsize, 0);<br>>> > +  if (off < 0) {<br>>> >            fprintf(stderr, _("existing superblock read failed: %s\n"),<br>>> >                    strerror(errno));<br>>> >            goto done;<br>>> >    }<br>>> > -  if (tmp != new_sb->sb_sectsize) {<br>>> > -          fprintf(stderr,<br>>> > -  _("warning: could not read existing superblock, skip zeroing\n"));<br>>> > +  /*<br>>> > +   * If we are creating an image file, it might be of zero length at this<br>>> > +   * point in time. Hence reading the existing superblock is going to<br>>> > +   * return zero bytes. It's not a failure we need to warn about in this<br>>> > +   * case.<br>>> > +   */<br>> except you already did "if (off < 0) fail" above this.<br><br></span><span style="color:rgb(34,34,34)">(oh, right, < 0 is different than == 0, sorry; so that part is ok)</span><br style="color:rgb(34,34,34)"><br style="color:rgb(34,34,34)"><span style="color:rgb(34,34,34)">Possibly better as:</span><br style="color:rgb(34,34,34)"><br style="color:rgb(34,34,34)"><span style="color:rgb(34,34,34)">if (off < 0 || (tmp != new_sb->sb_sectsize && !xi->disfile))</span><br style="color:rgb(34,34,34)"><span style="color:rgb(34,34,34)">        fprintf("error reading existing superblock ...")</span></div></div></blockquote><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​OK, better to be sure. :-)</div></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">​</div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5"><br style="color:rgb(34,34,34)"><br style="color:rgb(34,34,34)"><span style="color:rgb(34,34,34)">I still think this patch might need a reset though :)</span><br style="color:rgb(34,34,34)"><br style="color:rgb(34,34,34)"><span style="color:rgb(34,34,34)">Thanks,</span><br style="color:rgb(34,34,34)"><span style="color:rgb(34,34,34)">-Eric</span><br style="color:rgb(34,34,34)"><span style="color:rgb(34,34,34)"><br>> Ok, at this point I think it might be best to revert to Dave's original version.<br>><br>> If there were specific problems you were trying to address, can you point them out?<br></span></div></div></blockquote><div><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">​On few places, the original patch looked as if files had direct IO still enabled​ (using platform_findsize...), and I think it was causing some failures - fixing issues is why I did most of the changes.</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">I will revert to the original version and see what exactly fails. But with being Friday late afternoon, the results will be available the next week (Wednesday and further, all my courses at university are stuffed in Mon/Tue).</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Cheers,</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Jan</div><div> </div></div><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Jan Tulak<br></div><a href="mailto:jtulak@redhat.com" target="_blank">jtulak@redhat.com</a> / <a href="mailto:jan@tulak.me" target="_blank">jan@tulak.me</a></div></div></div></div>
</div></div>