| Message ID | 1481205380-22978-2-git-send-email-chandan@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
At 12/08/2016 09:56 PM, Chandan Rajendra wrote: > migrate_super_block() uses sectorsize to refer to the size of the > superblock. Hence on 64k sectorsize filesystems, it ends up computing > checksum beyond the super block length (i.e. > BTRFS_SUPER_INFO_SIZE). This commit fixes the bug by using > BTRFS_SUPER_INFO_SIZE instead of sectorsize of the underlying > filesystem. > > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com> Reviewed-by: Qu Wenruo <quwenruo@cn.fujitsu.com> BTW would you please enhance the convert tests? Current convert tests only uses 4K as block size. So adding 64K blocksize would definitely improve the tests. Thanks, Qu > --- > convert/main.c | 23 ++++++++++++----------- > 1 file changed, 12 insertions(+), 11 deletions(-) > > diff --git a/convert/main.c b/convert/main.c > index 1148a36..fd6f77b 100644 > --- a/convert/main.c > +++ b/convert/main.c > @@ -1360,7 +1360,7 @@ err: > /* > * Migrate super block to its default position and zero 0 ~ 16k > */ > -static int migrate_super_block(int fd, u64 old_bytenr, u32 sectorsize) > +static int migrate_super_block(int fd, u64 old_bytenr) > { > int ret; > struct extent_buffer *buf; > @@ -1368,13 +1368,13 @@ static int migrate_super_block(int fd, u64 old_bytenr, u32 sectorsize) > u32 len; > u32 bytenr; > > - buf = malloc(sizeof(*buf) + sectorsize); > + buf = malloc(sizeof(*buf) + BTRFS_SUPER_INFO_SIZE); > if (!buf) > return -ENOMEM; > > - buf->len = sectorsize; > - ret = pread(fd, buf->data, sectorsize, old_bytenr); > - if (ret != sectorsize) > + buf->len = BTRFS_SUPER_INFO_SIZE; > + ret = pread(fd, buf->data, BTRFS_SUPER_INFO_SIZE, old_bytenr); > + if (ret != BTRFS_SUPER_INFO_SIZE) > goto fail; > > super = (struct btrfs_super_block *)buf->data; > @@ -1382,19 +1382,20 @@ static int migrate_super_block(int fd, u64 old_bytenr, u32 sectorsize) > btrfs_set_super_bytenr(super, BTRFS_SUPER_INFO_OFFSET); > > csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0); > - ret = pwrite(fd, buf->data, sectorsize, BTRFS_SUPER_INFO_OFFSET); > - if (ret != sectorsize) > + ret = pwrite(fd, buf->data, BTRFS_SUPER_INFO_SIZE, > + BTRFS_SUPER_INFO_OFFSET); > + if (ret != BTRFS_SUPER_INFO_SIZE) > goto fail; > > ret = fsync(fd); > if (ret) > goto fail; > > - memset(buf->data, 0, sectorsize); > + memset(buf->data, 0, BTRFS_SUPER_INFO_SIZE); > for (bytenr = 0; bytenr < BTRFS_SUPER_INFO_OFFSET; ) { > len = BTRFS_SUPER_INFO_OFFSET - bytenr; > - if (len > sectorsize) > - len = sectorsize; > + if (len > BTRFS_SUPER_INFO_SIZE) > + len = BTRFS_SUPER_INFO_SIZE; > ret = pwrite(fd, buf->data, len, bytenr); > if (ret != len) { > fprintf(stderr, "unable to zero fill device\n"); > @@ -2519,7 +2520,7 @@ static int do_convert(const char *devname, int datacsum, int packing, > * If this step succeed, we get a mountable btrfs. Otherwise > * the source fs is left unchanged. > */ > - ret = migrate_super_block(fd, mkfs_cfg.super_bytenr, blocksize); > + ret = migrate_super_block(fd, mkfs_cfg.super_bytenr); > if (ret) { > error("unable to migrate super block: %d", ret); > goto fail; > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, December 09, 2016 09:09:29 AM Qu Wenruo wrote: > > At 12/08/2016 09:56 PM, Chandan Rajendra wrote: > > migrate_super_block() uses sectorsize to refer to the size of the > > superblock. Hence on 64k sectorsize filesystems, it ends up computing > > checksum beyond the super block length (i.e. > > BTRFS_SUPER_INFO_SIZE). This commit fixes the bug by using > > BTRFS_SUPER_INFO_SIZE instead of sectorsize of the underlying > > filesystem. > > > > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com> > > Reviewed-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > > BTW would you please enhance the convert tests? > Current convert tests only uses 4K as block size. > So adding 64K blocksize would definitely improve the tests. > Thanks for the hint. I just executed btrfs/012 with 64k blocksize hardcoded and found that 'btrfs rollback' failed. I will fix rollback first and then work on getting btrfs/012 to support 64k blocksize.
On Fri, Dec 09, 2016 at 09:09:29AM +0800, Qu Wenruo wrote: > > > At 12/08/2016 09:56 PM, Chandan Rajendra wrote: > > migrate_super_block() uses sectorsize to refer to the size of the > > superblock. Hence on 64k sectorsize filesystems, it ends up computing > > checksum beyond the super block length (i.e. > > BTRFS_SUPER_INFO_SIZE). This commit fixes the bug by using > > BTRFS_SUPER_INFO_SIZE instead of sectorsize of the underlying > > filesystem. > > > > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com> > > Reviewed-by: Qu Wenruo <quwenruo@cn.fujitsu.com> Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/convert/main.c b/convert/main.c index 1148a36..fd6f77b 100644 --- a/convert/main.c +++ b/convert/main.c @@ -1360,7 +1360,7 @@ err: /* * Migrate super block to its default position and zero 0 ~ 16k */ -static int migrate_super_block(int fd, u64 old_bytenr, u32 sectorsize) +static int migrate_super_block(int fd, u64 old_bytenr) { int ret; struct extent_buffer *buf; @@ -1368,13 +1368,13 @@ static int migrate_super_block(int fd, u64 old_bytenr, u32 sectorsize) u32 len; u32 bytenr; - buf = malloc(sizeof(*buf) + sectorsize); + buf = malloc(sizeof(*buf) + BTRFS_SUPER_INFO_SIZE); if (!buf) return -ENOMEM; - buf->len = sectorsize; - ret = pread(fd, buf->data, sectorsize, old_bytenr); - if (ret != sectorsize) + buf->len = BTRFS_SUPER_INFO_SIZE; + ret = pread(fd, buf->data, BTRFS_SUPER_INFO_SIZE, old_bytenr); + if (ret != BTRFS_SUPER_INFO_SIZE) goto fail; super = (struct btrfs_super_block *)buf->data; @@ -1382,19 +1382,20 @@ static int migrate_super_block(int fd, u64 old_bytenr, u32 sectorsize) btrfs_set_super_bytenr(super, BTRFS_SUPER_INFO_OFFSET); csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0); - ret = pwrite(fd, buf->data, sectorsize, BTRFS_SUPER_INFO_OFFSET); - if (ret != sectorsize) + ret = pwrite(fd, buf->data, BTRFS_SUPER_INFO_SIZE, + BTRFS_SUPER_INFO_OFFSET); + if (ret != BTRFS_SUPER_INFO_SIZE) goto fail; ret = fsync(fd); if (ret) goto fail; - memset(buf->data, 0, sectorsize); + memset(buf->data, 0, BTRFS_SUPER_INFO_SIZE); for (bytenr = 0; bytenr < BTRFS_SUPER_INFO_OFFSET; ) { len = BTRFS_SUPER_INFO_OFFSET - bytenr; - if (len > sectorsize) - len = sectorsize; + if (len > BTRFS_SUPER_INFO_SIZE) + len = BTRFS_SUPER_INFO_SIZE; ret = pwrite(fd, buf->data, len, bytenr); if (ret != len) { fprintf(stderr, "unable to zero fill device\n"); @@ -2519,7 +2520,7 @@ static int do_convert(const char *devname, int datacsum, int packing, * If this step succeed, we get a mountable btrfs. Otherwise * the source fs is left unchanged. */ - ret = migrate_super_block(fd, mkfs_cfg.super_bytenr, blocksize); + ret = migrate_super_block(fd, mkfs_cfg.super_bytenr); if (ret) { error("unable to migrate super block: %d", ret); goto fail;
migrate_super_block() uses sectorsize to refer to the size of the superblock. Hence on 64k sectorsize filesystems, it ends up computing checksum beyond the super block length (i.e. BTRFS_SUPER_INFO_SIZE). This commit fixes the bug by using BTRFS_SUPER_INFO_SIZE instead of sectorsize of the underlying filesystem. Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com> --- convert/main.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)