* [PATCH] btrfs: Remove WARN_ON for unaligned device created before v4.13 and adds more user friendly output
@ 2017-09-20 6:18 Qu Wenruo
2017-09-23 1:19 ` Satoru Takeuchi
0 siblings, 1 reply; 5+ messages in thread
From: Qu Wenruo @ 2017-09-20 6:18 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba, rich, rrauenza
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");
+ }
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs: Remove WARN_ON for unaligned device created before v4.13 and adds more user friendly output
2017-09-20 6:18 [PATCH] btrfs: Remove WARN_ON for unaligned device created before v4.13 and adds more user friendly output Qu Wenruo
@ 2017-09-23 1:19 ` Satoru Takeuchi
2017-09-23 1:27 ` Satoru Takeuchi
0 siblings, 1 reply; 5+ messages in thread
From: Satoru Takeuchi @ 2017-09-23 1:19 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs, dsterba, rich, rrauenza
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs: Remove WARN_ON for unaligned device created before v4.13 and adds more user friendly output
2017-09-23 1:19 ` Satoru Takeuchi
@ 2017-09-23 1:27 ` Satoru Takeuchi
2017-09-23 7:22 ` Qu Wenruo
0 siblings, 1 reply; 5+ messages in thread
From: Satoru Takeuchi @ 2017-09-23 1:27 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs, dsterba, rich, rrauenza
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs: Remove WARN_ON for unaligned device created before v4.13 and adds more user friendly output
2017-09-23 1:27 ` Satoru Takeuchi
@ 2017-09-23 7:22 ` Qu Wenruo
2017-10-19 17:50 ` David Sterba
0 siblings, 1 reply; 5+ messages in thread
From: Qu Wenruo @ 2017-09-23 7:22 UTC (permalink / raw)
To: Satoru Takeuchi; +Cc: linux-btrfs, dsterba, rich, rrauenza
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs: Remove WARN_ON for unaligned device created before v4.13 and adds more user friendly output
2017-09-23 7:22 ` Qu Wenruo
@ 2017-10-19 17:50 ` David Sterba
0 siblings, 0 replies; 5+ messages in thread
From: David Sterba @ 2017-10-19 17:50 UTC (permalink / raw)
To: Qu Wenruo; +Cc: Satoru Takeuchi, linux-btrfs, dsterba, rich, rrauenza
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-10-19 17:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-20 6:18 [PATCH] btrfs: Remove WARN_ON for unaligned device created before v4.13 and adds more user friendly output Qu Wenruo
2017-09-23 1:19 ` Satoru Takeuchi
2017-09-23 1:27 ` Satoru Takeuchi
2017-09-23 7:22 ` Qu Wenruo
2017-10-19 17:50 ` David Sterba
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.