* Re: [PATCH v2] Btrfs: fix unexpected balance crash due to BUG_ON
2016-07-11 17:21 [PATCH v2] Btrfs: fix unexpected balance crash due to BUG_ON Liu Bo
@ 2016-07-11 20:39 ` Chris Mason
2016-07-12 8:45 ` Filipe Manana
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Chris Mason @ 2016-07-11 20:39 UTC (permalink / raw)
To: Liu Bo, linux-btrfs; +Cc: David Sterba
On 07/11/2016 01:21 PM, Liu Bo wrote:
> Mounting a btrfs can resume previous balance operations asynchronously.
> An user got a crash when one drive has some corrupt sectors.
>
> Since balance can cancel itself in case of any error, we can gracefully
> return errors to upper layers and let balance do the cancel job.
>
> Reported-by: sash <master.b.at.raven@chefmail.de>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
v2 Looks good, thanks Liu.
Signed-off-by: Chris Mason <clm@fb.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] Btrfs: fix unexpected balance crash due to BUG_ON
2016-07-11 17:21 [PATCH v2] Btrfs: fix unexpected balance crash due to BUG_ON Liu Bo
2016-07-11 20:39 ` Chris Mason
@ 2016-07-12 8:45 ` Filipe Manana
2016-07-12 12:02 ` David Sterba
2016-07-12 18:24 ` [PATCH v3] " Liu Bo
3 siblings, 0 replies; 6+ messages in thread
From: Filipe Manana @ 2016-07-12 8:45 UTC (permalink / raw)
To: Liu Bo; +Cc: linux-btrfs@vger.kernel.org, David Sterba
On Mon, Jul 11, 2016 at 6:21 PM, Liu Bo <bo.li.liu@oracle.com> wrote:
> Mounting a btrfs can resume previous balance operations asynchronously.
> An user got a crash when one drive has some corrupt sectors.
>
> Since balance can cancel itself in case of any error, we can gracefully
> return errors to upper layers and let balance do the cancel job.
>
> Reported-by: sash <master.b.at.raven@chefmail.de>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
> v2: - Initialize path with NULL.
> - Show more information when we bail out.
>
> fs/btrfs/volumes.c | 30 ++++++++++++++++++++++++++----
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 589f128..348a183 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3421,7 +3421,7 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
> u64 size_to_free;
> u64 chunk_type;
> struct btrfs_chunk *chunk;
> - struct btrfs_path *path;
> + struct btrfs_path *path = NULL;
> struct btrfs_key key;
> struct btrfs_key found_key;
> struct btrfs_trans_handle *trans;
> @@ -3455,13 +3455,35 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
> ret = btrfs_shrink_device(device, old_size - size_to_free);
> if (ret == -ENOSPC)
> break;
> - BUG_ON(ret);
> + if (ret) {
> + /* btrfs_shrink_device never returns ret > 0 */
> + WARN_ON(ret > 0);
> + goto error;
> + }
>
> trans = btrfs_start_transaction(dev_root, 0);
> - BUG_ON(IS_ERR(trans));
> + if (IS_ERR(trans)) {
> + ret = PTR_ERR(trans);
> + btrfs_info(fs_info,
> + "%s:%d fails on btrfs_start_transaction() right after shrinking devivce %s (original size is %llu new size is %llu",
devivce -> device
> + __func__, __LINE__,
> + rcu_str_deref(device->name), old_size,
> + old_size - size_to_free);
> + goto error;
> + }
>
> ret = btrfs_grow_device(trans, device, old_size);
> - BUG_ON(ret);
> + if (ret) {
> + btrfs_end_transaction(trans, dev_root);
> + /* btrfs_grow_device never returns ret > 0 */
> + WARN_ON(ret > 0);
> + btrfs_info(fs_info,
> + "%s:%d fails on btrfs_grow_device() right after shrinking devivce %s (original size is %llu new size is %llu",
devivce -> device
> + __func__, __LINE__,
> + rcu_str_deref(device->name), old_size,
> + old_size - size_to_free);
> + goto error;
> + }
>
> btrfs_end_transaction(trans, dev_root);
> }
> --
> 2.5.5
>
> --
> 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
--
Filipe David Manana,
"People will forget what you said,
people will forget what you did,
but people will never forget how you made them feel."
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] Btrfs: fix unexpected balance crash due to BUG_ON
2016-07-11 17:21 [PATCH v2] Btrfs: fix unexpected balance crash due to BUG_ON Liu Bo
2016-07-11 20:39 ` Chris Mason
2016-07-12 8:45 ` Filipe Manana
@ 2016-07-12 12:02 ` David Sterba
2016-07-12 18:24 ` [PATCH v3] " Liu Bo
3 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2016-07-12 12:02 UTC (permalink / raw)
To: Liu Bo; +Cc: linux-btrfs
On Mon, Jul 11, 2016 at 10:21:35AM -0700, Liu Bo wrote:
> + }
>
> trans = btrfs_start_transaction(dev_root, 0);
> - BUG_ON(IS_ERR(trans));
> + if (IS_ERR(trans)) {
> + ret = PTR_ERR(trans);
> + btrfs_info(fs_info,
This could be btrfs_info_in_rcu for clarity (using the rcu_string)
> + "%s:%d fails on btrfs_start_transaction() right after shrinking devivce %s (original size is %llu new size is %llu",
> + __func__, __LINE__,
I'm not sure the function and line is necessary, we don't use it
anywhere else. I'd suggest a slight modification:
"resize: unable to start transaction after shrinking device %s (error %d), old size %llu, new size %llu"
> + rcu_str_deref(device->name), old_size,
> + old_size - size_to_free);
> + goto error;
> + }
>
> ret = btrfs_grow_device(trans, device, old_size);
> - BUG_ON(ret);
> + if (ret) {
> + btrfs_end_transaction(trans, dev_root);
> + /* btrfs_grow_device never returns ret > 0 */
> + WARN_ON(ret > 0);
> + btrfs_info(fs_info,
> + "%s:%d fails on btrfs_grow_device() right after shrinking devivce %s (original size is %llu new size is %llu",
"resize: unable to grow device after shrinking device %s (error %d), old size %llu, new size %llu"
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v3] Btrfs: fix unexpected balance crash due to BUG_ON
2016-07-11 17:21 [PATCH v2] Btrfs: fix unexpected balance crash due to BUG_ON Liu Bo
` (2 preceding siblings ...)
2016-07-12 12:02 ` David Sterba
@ 2016-07-12 18:24 ` Liu Bo
2016-07-15 15:38 ` David Sterba
3 siblings, 1 reply; 6+ messages in thread
From: Liu Bo @ 2016-07-12 18:24 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba, Chris Mason, Filipe Manana
Mounting a btrfs can resume previous balance operations asynchronously.
An user got a crash when one drive has some corrupt sectors.
Since balance can cancel itself in case of any error, we can gracefully
return errors to upper layers and let balance do the cancel job.
Reported-by: sash <master.b.at.raven@chefmail.de>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
v2: - Initialize path with NULL.
- Show more information when we bail out.
v3: - Fix typo
- Use rcu version btrfs_info
- Remove __func__ and __LINE__
fs/btrfs/volumes.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 589f128..6040c26 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3421,7 +3421,7 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
u64 size_to_free;
u64 chunk_type;
struct btrfs_chunk *chunk;
- struct btrfs_path *path;
+ struct btrfs_path *path = NULL;
struct btrfs_key key;
struct btrfs_key found_key;
struct btrfs_trans_handle *trans;
@@ -3455,13 +3455,33 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
ret = btrfs_shrink_device(device, old_size - size_to_free);
if (ret == -ENOSPC)
break;
- BUG_ON(ret);
+ if (ret) {
+ /* btrfs_shrink_device never returns ret > 0 */
+ WARN_ON(ret > 0);
+ goto error;
+ }
trans = btrfs_start_transaction(dev_root, 0);
- BUG_ON(IS_ERR(trans));
+ if (IS_ERR(trans)) {
+ ret = PTR_ERR(trans);
+ btrfs_info_in_rcu(fs_info,
+ "resize: unable to start transaction after shrinking device %s (error %d), old size %llu, new size %llu",
+ rcu_str_deref(device->name), ret,
+ old_size, old_size - size_to_free);
+ goto error;
+ }
ret = btrfs_grow_device(trans, device, old_size);
- BUG_ON(ret);
+ if (ret) {
+ btrfs_end_transaction(trans, dev_root);
+ /* btrfs_grow_device never returns ret > 0 */
+ WARN_ON(ret > 0);
+ btrfs_info_in_rcu(fs_info,
+ "resize: unable to grow device after shrinking device %s (error %d), old size %llu, new size %llu",
+ rcu_str_deref(device->name), ret,
+ old_size, old_size - size_to_free);
+ goto error;
+ }
btrfs_end_transaction(trans, dev_root);
}
--
2.5.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3] Btrfs: fix unexpected balance crash due to BUG_ON
2016-07-12 18:24 ` [PATCH v3] " Liu Bo
@ 2016-07-15 15:38 ` David Sterba
0 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2016-07-15 15:38 UTC (permalink / raw)
To: Liu Bo; +Cc: linux-btrfs, David Sterba, Chris Mason, Filipe Manana
On Tue, Jul 12, 2016 at 11:24:21AM -0700, Liu Bo wrote:
> Mounting a btrfs can resume previous balance operations asynchronously.
> An user got a crash when one drive has some corrupt sectors.
>
> Since balance can cancel itself in case of any error, we can gracefully
> return errors to upper layers and let balance do the cancel job.
>
> Reported-by: sash <master.b.at.raven@chefmail.de>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
Reviewed-by: David Sterba <dsterba@suse.com>
^ permalink raw reply [flat|nested] 6+ messages in thread