* [[PATCH 2/2 v2] xfs_repair: new secondary superblock search method] xfs_repair: new secondary superblock search method @ 2016-02-10 1:34 Bill O'Donnell 2016-02-10 4:49 ` [PATCH " Eric Sandeen 0 siblings, 1 reply; 4+ messages in thread From: Bill O'Donnell @ 2016-02-10 1:34 UTC (permalink / raw) To: xfs Optimize secondary sb search, using similar method to find fs geometry as that of xfs_mkfs. If this faster method fails in finding a secondary sb, fall back to original brute force slower search. Signed-off-by: Bill O'Donnell <billodo@redhat.com> --- Makefile | 2 +- include/libxcmd.h | 4 +++- libxcmd/topology.c | 35 ++++++++++++++++++++++++++++---- repair/Makefile | 4 ++-- repair/sb.c | 58 ++++++++++++++++++++++++++++++++++++++++++++---------- 5 files changed, 85 insertions(+), 18 deletions(-) diff --git a/Makefile b/Makefile index fca0a42..1d60d9c 100644 --- a/Makefile +++ b/Makefile @@ -80,7 +80,7 @@ fsr: libhandle growfs: libxcmd io: libxcmd libhandle quota: libxcmd -repair: libxlog +repair: libxlog libxcmd copy: libxlog ifeq ($(HAVE_BUILDDEFS), yes) diff --git a/include/libxcmd.h b/include/libxcmd.h index df7046e..b140adb 100644 --- a/include/libxcmd.h +++ b/include/libxcmd.h @@ -50,6 +50,8 @@ extern int check_overwrite( char *device); - +extern int guess_default_geometry(__uint64_t *agsize, + __uint64_t *agcount, + libxfs_init_t x); #endif /* __LIBXCMD_H__ */ diff --git a/libxcmd/topology.c b/libxcmd/topology.c index 0eeea28..b98d9b9 100644 --- a/libxcmd/topology.c +++ b/libxcmd/topology.c @@ -192,7 +192,8 @@ out: return ret; } -static void blkid_get_topology( +static void +blkid_get_topology( const char *device, int *sunit, int *swidth, @@ -284,7 +285,8 @@ check_overwrite( return 1; } -static void blkid_get_topology( +static void +blkid_get_topology( const char *device, int *sunit, int *swidth, @@ -302,8 +304,8 @@ static void blkid_get_topology( #endif /* ENABLE_BLKID */ - -void get_topology( +void +get_topology( libxfs_init_t *xi, struct fs_topology *ft, int force_overwrite) @@ -346,3 +348,28 @@ void get_topology( &lsectorsize, &psectorsize, force_overwrite); } } + +int +guess_default_geometry(__uint64_t *agsize, __uint64_t *agcount, + libxfs_init_t x) +{ + struct fs_topology ft; + int blocklog; + __uint64_t dblocks; + int multidisk; + + memset(&ft, 0, sizeof(ft)); + get_topology(&x, &ft, 1); + + /* + * get geometry from get_topology result. + * Use default block size (2^12) + */ + blocklog = 12; + multidisk = ft.dswidth | ft.dsunit; + dblocks = x.dsize >> (blocklog - BBSHIFT); + calc_default_ag_geometry(blocklog, dblocks, multidisk, + agsize, agcount); + + return blocklog; +} diff --git a/repair/Makefile b/repair/Makefile index 251722b..d24ab1f 100644 --- a/repair/Makefile +++ b/repair/Makefile @@ -20,8 +20,8 @@ CFILES = agheader.c attr_repair.c avl.c avl64.c bmap.c btree.c \ progress.c prefetch.c rt.c sb.c scan.c threads.c \ versions.c xfs_repair.c -LLDLIBS = $(LIBXFS) $(LIBXLOG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD) -LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG) +LLDLIBS = $(LIBBLKID) $(LIBXFS) $(LIBXLOG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD) $(LIBXCMD) +LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG) $(LIBXCMD) LLDFLAGS = -static-libtool-libs default: depend $(LTCOMMAND) diff --git a/repair/sb.c b/repair/sb.c index 4eef14a..8bc246e 100644 --- a/repair/sb.c +++ b/repair/sb.c @@ -22,6 +22,7 @@ #include "globals.h" #include "protos.h" #include "err_protos.h" +#include "libxcmd.h" #define BSIZE (1024 * 1024) @@ -85,10 +86,11 @@ copy_sb(xfs_sb_t *source, xfs_sb_t *dest) } /* - * find a secondary superblock, copy it into the sb buffer + * find a secondary superblock, copy it into the sb buffer. + * skipsize units is bytes, it contains either the agsize in bytes + * (if known), or the minimum agsize in bytes if agsize unknown. */ -int -find_secondary_sb(xfs_sb_t *rsb) +static int __find_secondary_sb(xfs_sb_t *rsb, __uint64_t skipsize) { xfs_off_t off; xfs_sb_t *sb; @@ -99,9 +101,9 @@ find_secondary_sb(xfs_sb_t *rsb) int dirty; int retval; int bsize; + int readsize; do_warn(_("\nattempting to find secondary superblock...\n")); - sb = (xfs_sb_t *)memalign(libxfs_device_alignment(), BSIZE); if (!sb) { do_error( @@ -113,13 +115,16 @@ find_secondary_sb(xfs_sb_t *rsb) retval = 0; dirty = 0; bsize = 0; + readsize = 0; /* * skip first sector since we know that's bad */ - for (done = 0, off = XFS_AG_MIN_BYTES; !done ; off += bsize) { + for (done = 0, off = skipsize; !done ; off += readsize) { /* - * read disk 1 MByte at a time. + * read disk using readsize interval + * (either the bytecount of actual agsize or bsize if + * agsize undetermined.) */ if (lseek64(x.dfd, off, SEEK_SET) != off) { done = 1; @@ -128,9 +133,13 @@ find_secondary_sb(xfs_sb_t *rsb) if (!done && (bsize = read(x.dfd, sb, BSIZE)) <= 0) { done = 1; } - + if (skipsize == XFS_AG_MIN_BYTES) { + readsize = bsize; + } + else { + readsize = skipsize; + } do_warn("."); - /* * check the buffer 512 bytes at a time since * we don't know how big the sectors really are. @@ -164,9 +173,38 @@ find_secondary_sb(xfs_sb_t *rsb) } } } - free(sb); - return(retval); + return retval; +} + +int +find_secondary_sb(xfs_sb_t *rsb) +{ + int retval; + __uint64_t skipsize; + __uint64_t agcount; + __uint64_t agsize; + int blocklog; + + /* + * Attempt to find secondary sb with a coarse approach, + * using a large skipsize (agsize in bytes). Failing that, + * fallback to the fine-grained approach using min agsize. + */ + blocklog = guess_default_geometry(&agsize, &agcount, x); + + /* + * use found ag geometry to quickly find secondary sb + */ + skipsize = agsize << blocklog; + retval = __find_secondary_sb(rsb, skipsize); + if (!retval) { + /* + * fallback: use minimum agsize for skipsize + */ + retval = __find_secondary_sb(rsb, XFS_AG_MIN_BYTES); + } + return retval; } /* -- 2.5.0 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2 v2] xfs_repair: new secondary superblock search method] xfs_repair: new secondary superblock search method 2016-02-10 1:34 [[PATCH 2/2 v2] xfs_repair: new secondary superblock search method] xfs_repair: new secondary superblock search method Bill O'Donnell @ 2016-02-10 4:49 ` Eric Sandeen 2016-02-10 15:43 ` Bill O'Donnell 0 siblings, 1 reply; 4+ messages in thread From: Eric Sandeen @ 2016-02-10 4:49 UTC (permalink / raw) To: xfs On 2/9/16 7:34 PM, Bill O'Donnell wrote: > Optimize secondary sb search, using similar method to find > fs geometry as that of xfs_mkfs. If this faster method fails > in finding a secondary sb, fall back to original brute force > slower search. > > Signed-off-by: Bill O'Donnell <billodo@redhat.com> > --- > Makefile | 2 +- > include/libxcmd.h | 4 +++- > libxcmd/topology.c | 35 ++++++++++++++++++++++++++++---- > repair/Makefile | 4 ++-- > repair/sb.c | 58 ++++++++++++++++++++++++++++++++++++++++++++---------- > 5 files changed, 85 insertions(+), 18 deletions(-) > > diff --git a/Makefile b/Makefile > index fca0a42..1d60d9c 100644 > --- a/Makefile > +++ b/Makefile > @@ -80,7 +80,7 @@ fsr: libhandle > growfs: libxcmd > io: libxcmd libhandle > quota: libxcmd > -repair: libxlog > +repair: libxlog libxcmd > copy: libxlog > > ifeq ($(HAVE_BUILDDEFS), yes) > diff --git a/include/libxcmd.h b/include/libxcmd.h > index df7046e..b140adb 100644 > --- a/include/libxcmd.h > +++ b/include/libxcmd.h > @@ -50,6 +50,8 @@ extern int > check_overwrite( > char *device); > > - > +extern int guess_default_geometry(__uint64_t *agsize, > + __uint64_t *agcount, > + libxfs_init_t x); > > #endif /* __LIBXCMD_H__ */ > diff --git a/libxcmd/topology.c b/libxcmd/topology.c > index 0eeea28..b98d9b9 100644 > --- a/libxcmd/topology.c > +++ b/libxcmd/topology.c > @@ -192,7 +192,8 @@ out: > return ret; > } > > -static void blkid_get_topology( > +static void > +blkid_get_topology( > const char *device, > int *sunit, > int *swidth, > @@ -284,7 +285,8 @@ check_overwrite( > return 1; > } > > -static void blkid_get_topology( > +static void > +blkid_get_topology( > const char *device, > int *sunit, > int *swidth, > @@ -302,8 +304,8 @@ static void blkid_get_topology( > > #endif /* ENABLE_BLKID */ > > - > -void get_topology( > +void > +get_topology( > libxfs_init_t *xi, > struct fs_topology *ft, > int force_overwrite) > @@ -346,3 +348,28 @@ void get_topology( > &lsectorsize, &psectorsize, force_overwrite); > } > } > + > +int > +guess_default_geometry(__uint64_t *agsize, __uint64_t *agcount, > + libxfs_init_t x) > +{ > + struct fs_topology ft; > + int blocklog; When in Rome: :) int guess_default_geometry( __uint64_t *agsize, __uint64_t *agcount, libxfs_init_t x) { ... (sorry, forgot to mention that last time, but other functions in the file are laid out this way; it is easier to read) > + __uint64_t dblocks; > + int multidisk; > + > + memset(&ft, 0, sizeof(ft)); > + get_topology(&x, &ft, 1); > + > + /* > + * get geometry from get_topology result. > + * Use default block size (2^12) > + */ > + blocklog = 12; > + multidisk = ft.dswidth | ft.dsunit; > + dblocks = x.dsize >> (blocklog - BBSHIFT); > + calc_default_ag_geometry(blocklog, dblocks, multidisk, > + agsize, agcount); > + > + return blocklog; > +} > diff --git a/repair/Makefile b/repair/Makefile > index 251722b..d24ab1f 100644 > --- a/repair/Makefile > +++ b/repair/Makefile > @@ -20,8 +20,8 @@ CFILES = agheader.c attr_repair.c avl.c avl64.c bmap.c btree.c \ > progress.c prefetch.c rt.c sb.c scan.c threads.c \ > versions.c xfs_repair.c > > -LLDLIBS = $(LIBXFS) $(LIBXLOG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD) > -LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG) > +LLDLIBS = $(LIBBLKID) $(LIBXFS) $(LIBXLOG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD) $(LIBXCMD) > +LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG) $(LIBXCMD) > LLDFLAGS = -static-libtool-libs > > default: depend $(LTCOMMAND) > diff --git a/repair/sb.c b/repair/sb.c > index 4eef14a..8bc246e 100644 > --- a/repair/sb.c > +++ b/repair/sb.c > @@ -22,6 +22,7 @@ > #include "globals.h" > #include "protos.h" > #include "err_protos.h" > +#include "libxcmd.h" Nitpick, we usually include that right after libxfs.h. > #define BSIZE (1024 * 1024) > > @@ -85,10 +86,11 @@ copy_sb(xfs_sb_t *source, xfs_sb_t *dest) > } > > /* > - * find a secondary superblock, copy it into the sb buffer > + * find a secondary superblock, copy it into the sb buffer. > + * skipsize units is bytes, it contains either the agsize in bytes > + * (if known), or the minimum agsize in bytes if agsize unknown. > */ Ok, so this is a little confused. There are actually 3 relevant parameters here: 1) The place to start reading 2) The number of bytes to read at that point 3) How far to seek forward for the next read In the old world, it's: 1) XFS_MIN_AG_BYTES 2) BSIZE (1MB) 3) BSIZE (i.e. it does sequential reads) In the new world, it's: 1) the guessed AG size (skipsize?) 2) BSIZE (more than we need, but not expensive, so leave it) 3) the guessed AG size (skipsize) You really have 2 different pieces of info: Where to do the first read, and how far forward to skip for the next read. I don't think you can convey that with a single argument, "skipsize." > -int > -find_secondary_sb(xfs_sb_t *rsb) > +static int __find_secondary_sb(xfs_sb_t *rsb, __uint64_t skipsize) better to format as: static int __find_secondary_sb( xfs_sb_t *rsb, __uint64_t skipsize) { but ... > { > xfs_off_t off; > xfs_sb_t *sb; > @@ -99,9 +101,9 @@ find_secondary_sb(xfs_sb_t *rsb) > int dirty; > int retval; > int bsize; > + int readsize; > > do_warn(_("\nattempting to find secondary superblock...\n")); > - > sb = (xfs_sb_t *)memalign(libxfs_device_alignment(), BSIZE); > if (!sb) { > do_error( > @@ -113,13 +115,16 @@ find_secondary_sb(xfs_sb_t *rsb) > retval = 0; > dirty = 0; > bsize = 0; > + readsize = 0; > > /* > * skip first sector since we know that's bad > */ > - for (done = 0, off = XFS_AG_MIN_BYTES; !done ; off += bsize) { > + for (done = 0, off = skipsize; !done ; off += readsize) { > /* > - * read disk 1 MByte at a time. we are actually still doing that, read(x.dfd, sb, BSIZE), so I'd leave the comment... > + * read disk using readsize interval > + * (either the bytecount of actual agsize or bsize if > + * agsize undetermined.) > */ > if (lseek64(x.dfd, off, SEEK_SET) != off) { > done = 1; > @@ -128,9 +133,13 @@ find_secondary_sb(xfs_sb_t *rsb) > if (!done && (bsize = read(x.dfd, sb, BSIZE)) <= 0) { > done = 1; > } > - > + if (skipsize == XFS_AG_MIN_BYTES) { > + readsize = bsize; > + } > + else { better as: } else { > + readsize = skipsize; > + } I wouldn't call this "readsize" - we are only ever reading BSIZE at a time. This is how much we advance each loop, so skipsize is a bit better. But the _functional_ problem here is that it is possible and permissible to have a filesystem with an AG size of XFS_AG_MIN_BYTES (16MB). So if you guessed a default geometry with an agsize of XFS_AG_MIN_BYTES, you've gone back to the fine-grained scanning. The fs probably isn't very big as a result, so it's not the end of the world, but ... see below. > do_warn("."); > - > /* > * check the buffer 512 bytes at a time since > * we don't know how big the sectors really are. > @@ -164,9 +173,38 @@ find_secondary_sb(xfs_sb_t *rsb) > } > } > } > - > free(sb); > - return(retval); > + return retval; > +} > + > +int > +find_secondary_sb(xfs_sb_t *rsb) > +{ > + int retval; > + __uint64_t skipsize; > + __uint64_t agcount; > + __uint64_t agsize; > + int blocklog; > + > + /* > + * Attempt to find secondary sb with a coarse approach, > + * using a large skipsize (agsize in bytes). Failing that, > + * fallback to the fine-grained approach using min agsize. > + */ > + blocklog = guess_default_geometry(&agsize, &agcount, x); > + > + /* > + * use found ag geometry to quickly find secondary sb > + */ > + skipsize = agsize << blocklog; > + retval = __find_secondary_sb(rsb, skipsize); > + if (!retval) { > + /* > + * fallback: use minimum agsize for skipsize > + */ > + retval = __find_secondary_sb(rsb, XFS_AG_MIN_BYTES); > + } > + return retval; > } I'd make __find_secondary_sb() take (sb, start, skip) i.e. send in this: > + retval = __find_secondary_sb(rsb, agsize, agsize); > + if (!retval) { > + /* > + * fallback: use minimum agsize for skipsize > + */ > + retval = __find_secondary_sb(rsb, XFS_AG_MIN_BYTES, BSIZE); > + } and the function is something like: static int __find_secondary_sb( xfs_sb_t *rsb, xfs_off_t start, xfs_off_t skip) { ... for (done = 0, off = start; !done ; off += skip) { ... if (lseek64(x.dfd, off, SEEK_SET) != off) done = 1; if (!done && (read(x.dfd, sb, BSIZE)) <= 0) done = 1; because you really can't deduce both the starting point and the skip-ahead size from just one parameter. -Eric _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2 v2] xfs_repair: new secondary superblock search method] xfs_repair: new secondary superblock search method 2016-02-10 4:49 ` [PATCH " Eric Sandeen @ 2016-02-10 15:43 ` Bill O'Donnell 2016-02-10 15:51 ` Eric Sandeen 0 siblings, 1 reply; 4+ messages in thread From: Bill O'Donnell @ 2016-02-10 15:43 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfs On Tue, Feb 09, 2016 at 10:49:20PM -0600, Eric Sandeen wrote: > On 2/9/16 7:34 PM, Bill O'Donnell wrote: > > Optimize secondary sb search, using similar method to find > > fs geometry as that of xfs_mkfs. If this faster method fails > > in finding a secondary sb, fall back to original brute force > > slower search. > > > > Signed-off-by: Bill O'Donnell <billodo@redhat.com> > > --- > > Makefile | 2 +- > > include/libxcmd.h | 4 +++- > > libxcmd/topology.c | 35 ++++++++++++++++++++++++++++---- > > repair/Makefile | 4 ++-- > > repair/sb.c | 58 ++++++++++++++++++++++++++++++++++++++++++++---------- > > 5 files changed, 85 insertions(+), 18 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index fca0a42..1d60d9c 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -80,7 +80,7 @@ fsr: libhandle > > growfs: libxcmd > > io: libxcmd libhandle > > quota: libxcmd > > -repair: libxlog > > +repair: libxlog libxcmd > > copy: libxlog > > > > ifeq ($(HAVE_BUILDDEFS), yes) > > diff --git a/include/libxcmd.h b/include/libxcmd.h > > index df7046e..b140adb 100644 > > --- a/include/libxcmd.h > > +++ b/include/libxcmd.h > > @@ -50,6 +50,8 @@ extern int > > check_overwrite( > > char *device); > > > > - > > +extern int guess_default_geometry(__uint64_t *agsize, > > + __uint64_t *agcount, > > + libxfs_init_t x); > > > > #endif /* __LIBXCMD_H__ */ > > diff --git a/libxcmd/topology.c b/libxcmd/topology.c > > index 0eeea28..b98d9b9 100644 > > --- a/libxcmd/topology.c > > +++ b/libxcmd/topology.c > > @@ -192,7 +192,8 @@ out: > > return ret; > > } > > > > -static void blkid_get_topology( > > +static void > > +blkid_get_topology( > > const char *device, > > int *sunit, > > int *swidth, > > @@ -284,7 +285,8 @@ check_overwrite( > > return 1; > > } > > > > -static void blkid_get_topology( > > +static void > > +blkid_get_topology( > > const char *device, > > int *sunit, > > int *swidth, > > @@ -302,8 +304,8 @@ static void blkid_get_topology( > > > > #endif /* ENABLE_BLKID */ > > > > - > > -void get_topology( > > +void > > +get_topology( > > libxfs_init_t *xi, > > struct fs_topology *ft, > > int force_overwrite) > > @@ -346,3 +348,28 @@ void get_topology( > > &lsectorsize, &psectorsize, force_overwrite); > > } > > } > > + > > +int > > +guess_default_geometry(__uint64_t *agsize, __uint64_t *agcount, > > + libxfs_init_t x) > > +{ > > + struct fs_topology ft; > > + int blocklog; > > When in Rome: :) > > int > guess_default_geometry( > __uint64_t *agsize, > __uint64_t *agcount, > libxfs_init_t x) > { > ... > > (sorry, forgot to mention that last time, but other functions in the > file are laid out this way; it is easier to read) > > > + __uint64_t dblocks; > > + int multidisk; > > + > > + memset(&ft, 0, sizeof(ft)); > > + get_topology(&x, &ft, 1); > > + > > + /* > > + * get geometry from get_topology result. > > + * Use default block size (2^12) > > + */ > > + blocklog = 12; > > + multidisk = ft.dswidth | ft.dsunit; > > + dblocks = x.dsize >> (blocklog - BBSHIFT); > > + calc_default_ag_geometry(blocklog, dblocks, multidisk, > > + agsize, agcount); > > + > > + return blocklog; > > +} > > diff --git a/repair/Makefile b/repair/Makefile > > index 251722b..d24ab1f 100644 > > --- a/repair/Makefile > > +++ b/repair/Makefile > > @@ -20,8 +20,8 @@ CFILES = agheader.c attr_repair.c avl.c avl64.c bmap.c btree.c \ > > progress.c prefetch.c rt.c sb.c scan.c threads.c \ > > versions.c xfs_repair.c > > > > -LLDLIBS = $(LIBXFS) $(LIBXLOG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD) > > -LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG) > > +LLDLIBS = $(LIBBLKID) $(LIBXFS) $(LIBXLOG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD) $(LIBXCMD) > > +LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG) $(LIBXCMD) > > LLDFLAGS = -static-libtool-libs > > > > default: depend $(LTCOMMAND) > > diff --git a/repair/sb.c b/repair/sb.c > > index 4eef14a..8bc246e 100644 > > --- a/repair/sb.c > > +++ b/repair/sb.c > > @@ -22,6 +22,7 @@ > > #include "globals.h" > > #include "protos.h" > > #include "err_protos.h" > > +#include "libxcmd.h" > > Nitpick, we usually include that right after libxfs.h. > > > #define BSIZE (1024 * 1024) > > > > @@ -85,10 +86,11 @@ copy_sb(xfs_sb_t *source, xfs_sb_t *dest) > > } > > > > /* > > - * find a secondary superblock, copy it into the sb buffer > > + * find a secondary superblock, copy it into the sb buffer. > > + * skipsize units is bytes, it contains either the agsize in bytes > > + * (if known), or the minimum agsize in bytes if agsize unknown. > > */ > > Ok, so this is a little confused. There are actually 3 relevant > parameters here: > > 1) The place to start reading > 2) The number of bytes to read at that point > 3) How far to seek forward for the next read > > In the old world, it's: > > 1) XFS_MIN_AG_BYTES > 2) BSIZE (1MB) > 3) BSIZE (i.e. it does sequential reads) > > In the new world, it's: > > 1) the guessed AG size (skipsize?) > 2) BSIZE (more than we need, but not expensive, so leave it) > 3) the guessed AG size (skipsize) > > You really have 2 different pieces of info: Where to do the first read, > and how far forward to skip for the next read. I don't think you > can convey that with a single argument, "skipsize." > > > -int > > -find_secondary_sb(xfs_sb_t *rsb) > > +static int __find_secondary_sb(xfs_sb_t *rsb, __uint64_t skipsize) > > better to format as: > > static int > __find_secondary_sb( > xfs_sb_t *rsb, > __uint64_t skipsize) > { > > but ... > > > { > > xfs_off_t off; > > xfs_sb_t *sb; > > @@ -99,9 +101,9 @@ find_secondary_sb(xfs_sb_t *rsb) > > int dirty; > > int retval; > > int bsize; > > + int readsize; > > > > do_warn(_("\nattempting to find secondary superblock...\n")); > > - > > sb = (xfs_sb_t *)memalign(libxfs_device_alignment(), BSIZE); > > if (!sb) { > > do_error( > > @@ -113,13 +115,16 @@ find_secondary_sb(xfs_sb_t *rsb) > > retval = 0; > > dirty = 0; > > bsize = 0; > > + readsize = 0; > > > > /* > > * skip first sector since we know that's bad > > */ > > - for (done = 0, off = XFS_AG_MIN_BYTES; !done ; off += bsize) { > > + for (done = 0, off = skipsize; !done ; off += readsize) { > > /* > > - * read disk 1 MByte at a time. > > we are actually still doing that, read(x.dfd, sb, BSIZE), so I'd > leave the comment... > > > + * read disk using readsize interval > > + * (either the bytecount of actual agsize or bsize if > > + * agsize undetermined.) > > > > */ > > if (lseek64(x.dfd, off, SEEK_SET) != off) { > > done = 1; > > @@ -128,9 +133,13 @@ find_secondary_sb(xfs_sb_t *rsb) > > if (!done && (bsize = read(x.dfd, sb, BSIZE)) <= 0) { > > done = 1; > > } > > - > > + if (skipsize == XFS_AG_MIN_BYTES) { > > + readsize = bsize; > > + } > > + else { > > better as: > } else { > > > + readsize = skipsize; > > + } > > I wouldn't call this "readsize" - we are only ever reading BSIZE at a time. > This is how much we advance each loop, so skipsize is a bit better. > > But the _functional_ problem here is that it is possible and permissible > to have a filesystem with an AG size of XFS_AG_MIN_BYTES (16MB). > > So if you guessed a default geometry with an agsize of XFS_AG_MIN_BYTES, > you've gone back to the fine-grained scanning. The fs probably isn't > very big as a result, so it's not the end of the world, but ... > see below. > > > do_warn("."); > > - > > /* > > * check the buffer 512 bytes at a time since > > * we don't know how big the sectors really are. > > @@ -164,9 +173,38 @@ find_secondary_sb(xfs_sb_t *rsb) > > } > > } > > } > > - > > free(sb); > > - return(retval); > > + return retval; > > +} > > + > > +int > > +find_secondary_sb(xfs_sb_t *rsb) > > +{ > > + int retval; > > + __uint64_t skipsize; > > + __uint64_t agcount; > > + __uint64_t agsize; > > + int blocklog; > > + > > + /* > > + * Attempt to find secondary sb with a coarse approach, > > + * using a large skipsize (agsize in bytes). Failing that, > > + * fallback to the fine-grained approach using min agsize. > > + */ > > + blocklog = guess_default_geometry(&agsize, &agcount, x); > > + > > + /* > > + * use found ag geometry to quickly find secondary sb > > + */ > > + skipsize = agsize << blocklog; > > + retval = __find_secondary_sb(rsb, skipsize); > > + if (!retval) { > > + /* > > + * fallback: use minimum agsize for skipsize > > + */ > > + retval = __find_secondary_sb(rsb, XFS_AG_MIN_BYTES); > > + } > > + return retval; > > } > > I'd make __find_secondary_sb() take (sb, start, skip) i.e. send in > this: > > > + retval = __find_secondary_sb(rsb, agsize, agsize); > > + if (!retval) { > > + /* > > + * fallback: use minimum agsize for skipsize > > + */ > > + retval = __find_secondary_sb(rsb, XFS_AG_MIN_BYTES, BSIZE); > > + } > > and the function is something like: > > static int > __find_secondary_sb( > xfs_sb_t *rsb, > xfs_off_t start, > xfs_off_t skip) > > { > > ... > > for (done = 0, off = start; !done ; off += skip) { > ... > if (lseek64(x.dfd, off, SEEK_SET) != off) > done = 1; > > if (!done && (read(x.dfd, sb, BSIZE)) <= 0) > done = 1; But, bsize is used here: ... * check the buffer 512 bytes at a time since * we don't know how big the sectors really are. */ for (i = 0; !done && i < bsize; i += BBSIZE) { ... so, don't we still need to populate bsize? Or does it make more sense to just use BBSIZE in the conditional, ala: for (i = 0; !done && i < BBSIZE; i += BBSIZE) > > > because you really can't deduce both the starting point and the skip-ahead > size from just one parameter. Agreed. Thanks for your thorough reviews :) Bill > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2 v2] xfs_repair: new secondary superblock search method] xfs_repair: new secondary superblock search method 2016-02-10 15:43 ` Bill O'Donnell @ 2016-02-10 15:51 ` Eric Sandeen 0 siblings, 0 replies; 4+ messages in thread From: Eric Sandeen @ 2016-02-10 15:51 UTC (permalink / raw) To: Bill O'Donnell; +Cc: xfs On 2/10/16 9:43 AM, Bill O'Donnell wrote: >> I'd make __find_secondary_sb() take (sb, start, skip) i.e. send in >> > this: >> > >>> > > + retval = __find_secondary_sb(rsb, agsize, agsize); >>> > > + if (!retval) { >>> > > + /* >>> > > + * fallback: use minimum agsize for skipsize >>> > > + */ >>> > > + retval = __find_secondary_sb(rsb, XFS_AG_MIN_BYTES, BSIZE); >>> > > + } >> > >> > and the function is something like: >> > >> > static int >> > __find_secondary_sb( >> > xfs_sb_t *rsb, >> > xfs_off_t start, >> > xfs_off_t skip) >> > >> > { >> > >> > ... >> > >> > for (done = 0, off = start; !done ; off += skip) { >> > ... >> > if (lseek64(x.dfd, off, SEEK_SET) != off) >> > done = 1; >> > >> > if (!done && (read(x.dfd, sb, BSIZE)) <= 0) >> > done = 1; > But, bsize is used here: > ... > * check the buffer 512 bytes at a time since > * we don't know how big the sectors really are. > */ > for (i = 0; !done && i < bsize; i += BBSIZE) { > ... > so, don't we still need to populate bsize? Or does it make more > sense to just use BBSIZE in the conditional, ala: > for (i = 0; !done && i < BBSIZE; i += BBSIZE) Oh, right, sorry. yes, keep the assignment: if (!done && (bsize = read(x.dfd, sb, BSIZE)) <= 0) { That handles a short read at the end; we ask for BSIZE, actually read bsize, and then we need to iterate over what actually got read (bsize). BSIZE, bsize ... clear as mud. :( -Eric >> > >> > >> > because you really can't deduce both the starting point and the skip-ahead >> > size from just one parameter. > Agreed. > Thanks for your thorough reviews :) > Bill > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-02-10 15:51 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-10 1:34 [[PATCH 2/2 v2] xfs_repair: new secondary superblock search method] xfs_repair: new secondary superblock search method Bill O'Donnell 2016-02-10 4:49 ` [PATCH " Eric Sandeen 2016-02-10 15:43 ` Bill O'Donnell 2016-02-10 15:51 ` Eric Sandeen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox