| Message ID | 20170920061843.9016-1-quwenruo.btrfs@gmx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
At Wed, 20 Sep 2017 15:18:43 +0900, Qu Wenruo wrote: > > Commit 7dfb8be11b5d ("btrfs: Round down values which are written for > total_bytes_size") is fixing the unaligned device size caused by > adding/shrinking device. > > It added a new WARN_ON() when device size is unaligned. > This is fine for new device added to btrfs using v4.13 kernel, but not > existing device whose total_bytes is already unaligned. > > And the WARN_ON() will get triggered every time a block group get > created/removed on the unaligned device. > > This patch will remove the WARN_ON(), and warn user more gently what's > happening and how to fix it. > > Reported-by: Rich Rauenzahn <rich@shroop.net> > Fixes: 7dfb8be11b5d ("btrfs: Round down values which are written for > total_bytes_size") > Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com> > --- > fs/btrfs/ctree.h | 1 - > fs/btrfs/volumes.c | 16 ++++++++++++---- > 2 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index 5a8933da39a7..4de9269e435a 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -1562,7 +1562,6 @@ static inline void btrfs_set_device_total_bytes(struct extent_buffer *eb, > { > BUILD_BUG_ON(sizeof(u64) != > sizeof(((struct btrfs_dev_item *)0))->total_bytes); > - WARN_ON(!IS_ALIGNED(val, eb->fs_info->sectorsize)); > btrfs_set_64(eb, s, offsetof(struct btrfs_dev_item, total_bytes), val); > } > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 0e8f16c305df..afae25df6a8c 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -6472,15 +6472,23 @@ static int read_one_chunk(struct btrfs_fs_info *fs_info, struct btrfs_key *key, > return 0; > } > > -static void fill_device_from_item(struct extent_buffer *leaf, > - struct btrfs_dev_item *dev_item, > - struct btrfs_device *device) > +static void fill_device_from_item(struct btrfs_fs_info *fs_info, > + struct extent_buffer *leaf, > + struct btrfs_dev_item *dev_item, > + struct btrfs_device *device) > { > unsigned long ptr; > > device->devid = btrfs_device_id(leaf, dev_item); > device->disk_total_bytes = btrfs_device_total_bytes(leaf, dev_item); > device->total_bytes = device->disk_total_bytes; > + if (!IS_ALIGNED(device->total_bytes, fs_info->sectorsize)) { > + btrfs_warn(fs_info, > + "devid %llu has unaligned total bytes %llu", > + device->devid, device->disk_total_bytes); > + btrfs_warn(fs_info, > + "please shrink the device a little and resize back to fix it"); > + } How about telling uses to know device->total_bytes should be alligned to fs_info->sectorsize here? Thanks, Satoru > device->commit_total_bytes = device->disk_total_bytes; > device->bytes_used = btrfs_device_bytes_used(leaf, dev_item); > device->commit_bytes_used = device->bytes_used; > @@ -6625,7 +6633,7 @@ static int read_one_dev(struct btrfs_fs_info *fs_info, > return -EINVAL; > } > > - fill_device_from_item(leaf, dev_item, device); > + fill_device_from_item(fs_info, leaf, dev_item, device); > device->in_fs_metadata = 1; > if (device->writeable && !device->is_tgtdev_for_dev_replace) { > device->fs_devices->total_rw_bytes += device->total_bytes; > -- > 2.14.1 > > -- > 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 -- 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
At Sat, 23 Sep 2017 10:19:26 +0900, Satoru Takeuchi wrote: > > At Wed, 20 Sep 2017 15:18:43 +0900, > Qu Wenruo wrote: > > > > Commit 7dfb8be11b5d ("btrfs: Round down values which are written for > > total_bytes_size") is fixing the unaligned device size caused by > > adding/shrinking device. > > > > It added a new WARN_ON() when device size is unaligned. > > This is fine for new device added to btrfs using v4.13 kernel, but not > > existing device whose total_bytes is already unaligned. > > > > And the WARN_ON() will get triggered every time a block group get > > created/removed on the unaligned device. > > > > This patch will remove the WARN_ON(), and warn user more gently what's > > happening and how to fix it. > > > > Reported-by: Rich Rauenzahn <rich@shroop.net> > > Fixes: 7dfb8be11b5d ("btrfs: Round down values which are written for > > total_bytes_size") > > Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com> > > --- > > fs/btrfs/ctree.h | 1 - > > fs/btrfs/volumes.c | 16 ++++++++++++---- > > 2 files changed, 12 insertions(+), 5 deletions(-) > > > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > > index 5a8933da39a7..4de9269e435a 100644 > > --- a/fs/btrfs/ctree.h > > +++ b/fs/btrfs/ctree.h > > @@ -1562,7 +1562,6 @@ static inline void btrfs_set_device_total_bytes(struct extent_buffer *eb, > > { > > BUILD_BUG_ON(sizeof(u64) != > > sizeof(((struct btrfs_dev_item *)0))->total_bytes); > > - WARN_ON(!IS_ALIGNED(val, eb->fs_info->sectorsize)); > > btrfs_set_64(eb, s, offsetof(struct btrfs_dev_item, total_bytes), val); > > } > > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > > index 0e8f16c305df..afae25df6a8c 100644 > > --- a/fs/btrfs/volumes.c > > +++ b/fs/btrfs/volumes.c > > @@ -6472,15 +6472,23 @@ static int read_one_chunk(struct btrfs_fs_info *fs_info, struct btrfs_key *key, > > return 0; > > } > > > > -static void fill_device_from_item(struct extent_buffer *leaf, > > - struct btrfs_dev_item *dev_item, > > - struct btrfs_device *device) > > +static void fill_device_from_item(struct btrfs_fs_info *fs_info, > > + struct extent_buffer *leaf, > > + struct btrfs_dev_item *dev_item, > > + struct btrfs_device *device) > > { > > unsigned long ptr; > > > > device->devid = btrfs_device_id(leaf, dev_item); > > device->disk_total_bytes = btrfs_device_total_bytes(leaf, dev_item); > > device->total_bytes = device->disk_total_bytes; > > + if (!IS_ALIGNED(device->total_bytes, fs_info->sectorsize)) { > > + btrfs_warn(fs_info, > > + "devid %llu has unaligned total bytes %llu", > > + device->devid, device->disk_total_bytes); > > + btrfs_warn(fs_info, > > + "please shrink the device a little and resize back to fix it"); > > + } > > How about telling uses to know device->total_bytes should be alligned > to fs_info->sectorsize here? > > Thanks, I should make my comment clearer, sorry. === + if (!IS_ALIGNED(device->total_bytes, fs_info->sectorsize)) { + btrfs_warn(fs_info, + "devid %llu: total bytes %llu should be aligned to %u bytes", + device->devid, device->disk_total_bytes, fs_info->sectorsize); + btrfs_warn(fs_info, + "please shrink the device a little and resize back to fix it"); + } === Thanks, Satoru > Satoru > > > device->commit_total_bytes = device->disk_total_bytes; > > device->bytes_used = btrfs_device_bytes_used(leaf, dev_item); > > device->commit_bytes_used = device->bytes_used; > > @@ -6625,7 +6633,7 @@ static int read_one_dev(struct btrfs_fs_info *fs_info, > > return -EINVAL; > > } > > > > - fill_device_from_item(leaf, dev_item, device); > > + fill_device_from_item(fs_info, leaf, dev_item, device); > > device->in_fs_metadata = 1; > > if (device->writeable && !device->is_tgtdev_for_dev_replace) { > > device->fs_devices->total_rw_bytes += device->total_bytes; > > -- > > 2.14.1 > > > > -- > > 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 -- 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 2017年09月23日 09:27, Satoru Takeuchi wrote: > At Sat, 23 Sep 2017 10:19:26 +0900, > Satoru Takeuchi wrote: >> >> At Wed, 20 Sep 2017 15:18:43 +0900, >> Qu Wenruo wrote: >>> >>> Commit 7dfb8be11b5d ("btrfs: Round down values which are written for >>> total_bytes_size") is fixing the unaligned device size caused by >>> adding/shrinking device. >>> >>> It added a new WARN_ON() when device size is unaligned. >>> This is fine for new device added to btrfs using v4.13 kernel, but not >>> existing device whose total_bytes is already unaligned. >>> >>> And the WARN_ON() will get triggered every time a block group get >>> created/removed on the unaligned device. >>> >>> This patch will remove the WARN_ON(), and warn user more gently what's >>> happening and how to fix it. >>> >>> Reported-by: Rich Rauenzahn <rich@shroop.net> >>> Fixes: 7dfb8be11b5d ("btrfs: Round down values which are written for >>> total_bytes_size") >>> Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com> >>> --- >>> fs/btrfs/ctree.h | 1 - >>> fs/btrfs/volumes.c | 16 ++++++++++++---- >>> 2 files changed, 12 insertions(+), 5 deletions(-) >>> >>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h >>> index 5a8933da39a7..4de9269e435a 100644 >>> --- a/fs/btrfs/ctree.h >>> +++ b/fs/btrfs/ctree.h >>> @@ -1562,7 +1562,6 @@ static inline void btrfs_set_device_total_bytes(struct extent_buffer *eb, >>> { >>> BUILD_BUG_ON(sizeof(u64) != >>> sizeof(((struct btrfs_dev_item *)0))->total_bytes); >>> - WARN_ON(!IS_ALIGNED(val, eb->fs_info->sectorsize)); >>> btrfs_set_64(eb, s, offsetof(struct btrfs_dev_item, total_bytes), val); >>> } >>> >>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>> index 0e8f16c305df..afae25df6a8c 100644 >>> --- a/fs/btrfs/volumes.c >>> +++ b/fs/btrfs/volumes.c >>> @@ -6472,15 +6472,23 @@ static int read_one_chunk(struct btrfs_fs_info *fs_info, struct btrfs_key *key, >>> return 0; >>> } >>> >>> -static void fill_device_from_item(struct extent_buffer *leaf, >>> - struct btrfs_dev_item *dev_item, >>> - struct btrfs_device *device) >>> +static void fill_device_from_item(struct btrfs_fs_info *fs_info, >>> + struct extent_buffer *leaf, >>> + struct btrfs_dev_item *dev_item, >>> + struct btrfs_device *device) >>> { >>> unsigned long ptr; >>> >>> device->devid = btrfs_device_id(leaf, dev_item); >>> device->disk_total_bytes = btrfs_device_total_bytes(leaf, dev_item); >>> device->total_bytes = device->disk_total_bytes; >>> + if (!IS_ALIGNED(device->total_bytes, fs_info->sectorsize)) { >>> + btrfs_warn(fs_info, >>> + "devid %llu has unaligned total bytes %llu", >>> + device->devid, device->disk_total_bytes); >>> + btrfs_warn(fs_info, >>> + "please shrink the device a little and resize back to fix it"); >>> + } >> >> How about telling uses to know device->total_bytes should be alligned >> to fs_info->sectorsize here? >> >> Thanks, > > I should make my comment clearer, sorry. > > === > + if (!IS_ALIGNED(device->total_bytes, fs_info->sectorsize)) { > + btrfs_warn(fs_info, > + "devid %llu: total bytes %llu should be aligned to %u bytes", > + device->devid, device->disk_total_bytes, fs_info->sectorsize); > + btrfs_warn(fs_info, > + "please shrink the device a little and resize back to fix it"); > + } > === That's better. But I'm also considering modifying the total_bytes directly here. So that any time DEV_ITEM and super block get updated, new aligned value will be written back to disk, and since the value is aligned in memory, it won't cause WARN_ON() any longer. I'll test and check the code for confirmation before updating the patch. Thanks, Qu > > Thanks, > Satoru > >> Satoru >> >>> device->commit_total_bytes = device->disk_total_bytes; >>> device->bytes_used = btrfs_device_bytes_used(leaf, dev_item); >>> device->commit_bytes_used = device->bytes_used; >>> @@ -6625,7 +6633,7 @@ static int read_one_dev(struct btrfs_fs_info *fs_info, >>> return -EINVAL; >>> } >>> >>> - fill_device_from_item(leaf, dev_item, device); >>> + fill_device_from_item(fs_info, leaf, dev_item, device); >>> device->in_fs_metadata = 1; >>> if (device->writeable && !device->is_tgtdev_for_dev_replace) { >>> device->fs_devices->total_rw_bytes += device->total_bytes; >>> -- >>> 2.14.1 >>> >>> -- >>> 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 -- 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 Sat, Sep 23, 2017 at 03:22:36PM +0800, Qu Wenruo wrote: > >>> --- a/fs/btrfs/volumes.c > >>> +++ b/fs/btrfs/volumes.c > >>> @@ -6472,15 +6472,23 @@ static int read_one_chunk(struct btrfs_fs_info *fs_info, struct btrfs_key *key, > >>> return 0; > >>> } > >>> > >>> -static void fill_device_from_item(struct extent_buffer *leaf, > >>> - struct btrfs_dev_item *dev_item, > >>> - struct btrfs_device *device) > >>> +static void fill_device_from_item(struct btrfs_fs_info *fs_info, > >>> + struct extent_buffer *leaf, > >>> + struct btrfs_dev_item *dev_item, > >>> + struct btrfs_device *device) > >>> { > >>> unsigned long ptr; > >>> > >>> device->devid = btrfs_device_id(leaf, dev_item); > >>> device->disk_total_bytes = btrfs_device_total_bytes(leaf, dev_item); > >>> device->total_bytes = device->disk_total_bytes; > >>> + if (!IS_ALIGNED(device->total_bytes, fs_info->sectorsize)) { > >>> + btrfs_warn(fs_info, > >>> + "devid %llu has unaligned total bytes %llu", > >>> + device->devid, device->disk_total_bytes); > >>> + btrfs_warn(fs_info, > >>> + "please shrink the device a little and resize back to fix it"); > >>> + } > >> > >> How about telling uses to know device->total_bytes should be alligned > >> to fs_info->sectorsize here? > >> > >> Thanks, > > > > I should make my comment clearer, sorry. > > > > === > > + if (!IS_ALIGNED(device->total_bytes, fs_info->sectorsize)) { > > + btrfs_warn(fs_info, > > + "devid %llu: total bytes %llu should be aligned to %u bytes", > > + device->devid, device->disk_total_bytes, fs_info->sectorsize); > > + btrfs_warn(fs_info, > > + "please shrink the device a little and resize back to fix it"); > > + } > > === > > That's better. > > But I'm also considering modifying the total_bytes directly here. Yeah, I think it would be better to fix here, without a warning even. The rounding error is below 4k and nodesize, we would never use this space for block groups so no accidental data loss. > So that any time DEV_ITEM and super block get updated, new aligned value > will be written back to disk, and since the value is aligned in memory, > it won't cause WARN_ON() any longer. > > I'll test and check the code for confirmation before updating the patch. -- 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/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 5a8933da39a7..4de9269e435a 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -1562,7 +1562,6 @@ static inline void btrfs_set_device_total_bytes(struct extent_buffer *eb, { BUILD_BUG_ON(sizeof(u64) != sizeof(((struct btrfs_dev_item *)0))->total_bytes); - WARN_ON(!IS_ALIGNED(val, eb->fs_info->sectorsize)); btrfs_set_64(eb, s, offsetof(struct btrfs_dev_item, total_bytes), val); } diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 0e8f16c305df..afae25df6a8c 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -6472,15 +6472,23 @@ static int read_one_chunk(struct btrfs_fs_info *fs_info, struct btrfs_key *key, return 0; } -static void fill_device_from_item(struct extent_buffer *leaf, - struct btrfs_dev_item *dev_item, - struct btrfs_device *device) +static void fill_device_from_item(struct btrfs_fs_info *fs_info, + struct extent_buffer *leaf, + struct btrfs_dev_item *dev_item, + struct btrfs_device *device) { unsigned long ptr; device->devid = btrfs_device_id(leaf, dev_item); device->disk_total_bytes = btrfs_device_total_bytes(leaf, dev_item); device->total_bytes = device->disk_total_bytes; + if (!IS_ALIGNED(device->total_bytes, fs_info->sectorsize)) { + btrfs_warn(fs_info, + "devid %llu has unaligned total bytes %llu", + device->devid, device->disk_total_bytes); + btrfs_warn(fs_info, + "please shrink the device a little and resize back to fix it"); + } device->commit_total_bytes = device->disk_total_bytes; device->bytes_used = btrfs_device_bytes_used(leaf, dev_item); device->commit_bytes_used = device->bytes_used; @@ -6625,7 +6633,7 @@ static int read_one_dev(struct btrfs_fs_info *fs_info, return -EINVAL; } - fill_device_from_item(leaf, dev_item, device); + fill_device_from_item(fs_info, leaf, dev_item, device); device->in_fs_metadata = 1; if (device->writeable && !device->is_tgtdev_for_dev_replace) { device->fs_devices->total_rw_bytes += device->total_bytes;
Commit 7dfb8be11b5d ("btrfs: Round down values which are written for total_bytes_size") is fixing the unaligned device size caused by adding/shrinking device. It added a new WARN_ON() when device size is unaligned. This is fine for new device added to btrfs using v4.13 kernel, but not existing device whose total_bytes is already unaligned. And the WARN_ON() will get triggered every time a block group get created/removed on the unaligned device. This patch will remove the WARN_ON(), and warn user more gently what's happening and how to fix it. Reported-by: Rich Rauenzahn <rich@shroop.net> Fixes: 7dfb8be11b5d ("btrfs: Round down values which are written for total_bytes_size") Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com> --- fs/btrfs/ctree.h | 1 - fs/btrfs/volumes.c | 16 ++++++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-)