* [PATCH] btrfs-progs: wipe copies of the stale superblock beyond -b size
@ 2018-03-26 8:27 Anand Jain
2018-03-26 19:25 ` Nikolay Borisov
0 siblings, 1 reply; 8+ messages in thread
From: Anand Jain @ 2018-03-26 8:27 UTC (permalink / raw)
To: linux-btrfs
During the mkfs.btrfs -b <blockcount> btrfs_prepare_device() zeros all
the superblock bytenr locations only if the bytenr is below the
blockcount. The problem with this is that if the BTRFS is recreated
with a smaller size then we will leave the stale superblock in the disk
which shall confuse the recovery. As shown in the test case below.
mkfs.btrfs -qf /dev/mapper/vg-lv
mkfs.btrfs -qf -b1G /dev/mapper/vg-lv
btrfs in dump-super -a /dev/mapper/vg-lv | grep '.fsid|superblock:'
superblock: bytenr=65536, device=/dev/mapper/vg-lv
dev_item.fsid ebc67d01-7fc5-43f0-90b4-d1925002551e [match]
superblock: bytenr=67108864, device=/dev/mapper/vg-lv
dev_item.fsid ebc67d01-7fc5-43f0-90b4-d1925002551e [match]
superblock: bytenr=274877906944, device=/dev/mapper/vg-lv
dev_item.fsid b97a9206-593b-4933-a424-c6a6ee23fe7c [match]
So if we find a valid superblock zero it even if it's beyond the
blockcount.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
utils.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/utils.c b/utils.c
index e9cb3a82fda6..6a9408b06e73 100644
--- a/utils.c
+++ b/utils.c
@@ -365,6 +365,41 @@ int btrfs_prepare_device(int fd, const char *file, u64 *block_count_ret,
return 1;
}
+ /*
+ * Check for the BTRFS SB copies up until btrfs_device_size() and zero
+ * it. So that kernel (or user for the manual recovery) don't have to
+ * confuse with the stale SB copy during recovery.
+ */
+ if (block_count != btrfs_device_size(fd, &st)) {
+ for (i = 1; i < BTRFS_SUPER_MIRROR_MAX; i++) {
+ struct btrfs_super_block *disk_super;
+ char buf[BTRFS_SUPER_INFO_SIZE];
+ disk_super = (struct btrfs_super_block *)buf;
+
+ /* Already zeroed above */
+ if (btrfs_sb_offset(i) < block_count)
+ continue;
+
+ /* Beyond actual disk size */
+ if (btrfs_sb_offset(i) >= btrfs_device_size(fd, &st))
+ continue;
+
+ /* Does not contain any stale SB */
+ if (btrfs_read_dev_super(fd, disk_super,
+ btrfs_sb_offset(i), 0))
+ continue;
+
+ ret = zero_dev_clamped(fd, btrfs_sb_offset(i),
+ BTRFS_SUPER_INFO_SIZE,
+ btrfs_device_size(fd, &st));
+ if (ret < 0) {
+ error("failed to zero device '%s' bytenr %llu: %s",
+ file, btrfs_sb_offset(i), strerror(-ret));
+ return 1;
+ }
+ }
+ }
+
*block_count_ret = block_count;
return 0;
}
--
2.7.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] btrfs-progs: wipe copies of the stale superblock beyond -b size
2018-03-26 8:27 [PATCH] btrfs-progs: wipe copies of the stale superblock beyond -b size Anand Jain
@ 2018-03-26 19:25 ` Nikolay Borisov
2018-03-26 23:50 ` Anand Jain
0 siblings, 1 reply; 8+ messages in thread
From: Nikolay Borisov @ 2018-03-26 19:25 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
On 26.03.2018 11:27, Anand Jain wrote:
> During the mkfs.btrfs -b <blockcount> btrfs_prepare_device() zeros all
> the superblock bytenr locations only if the bytenr is below the
> blockcount. The problem with this is that if the BTRFS is recreated
> with a smaller size then we will leave the stale superblock in the disk
> which shall confuse the recovery. As shown in the test case below.
>
> mkfs.btrfs -qf /dev/mapper/vg-lv
> mkfs.btrfs -qf -b1G /dev/mapper/vg-lv
> btrfs in dump-super -a /dev/mapper/vg-lv | grep '.fsid|superblock:'
>
> superblock: bytenr=65536, device=/dev/mapper/vg-lv
> dev_item.fsid ebc67d01-7fc5-43f0-90b4-d1925002551e [match]
> superblock: bytenr=67108864, device=/dev/mapper/vg-lv
> dev_item.fsid ebc67d01-7fc5-43f0-90b4-d1925002551e [match]
> superblock: bytenr=274877906944, device=/dev/mapper/vg-lv
> dev_item.fsid b97a9206-593b-4933-a424-c6a6ee23fe7c [match]
>
> So if we find a valid superblock zero it even if it's beyond the
> blockcount.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> utils.c | 35 +++++++++++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/utils.c b/utils.c
> index e9cb3a82fda6..6a9408b06e73 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -365,6 +365,41 @@ int btrfs_prepare_device(int fd, const char *file, u64 *block_count_ret,
> return 1;
> }
>
> + /*
> + * Check for the BTRFS SB copies up until btrfs_device_size() and zero
> + * it. So that kernel (or user for the manual recovery) don't have to
> + * confuse with the stale SB copy during recovery.
> + */
> + if (block_count != btrfs_device_size(fd, &st)) {
> + for (i = 1; i < BTRFS_SUPER_MIRROR_MAX; i++) {
> + struct btrfs_super_block *disk_super;
> + char buf[BTRFS_SUPER_INFO_SIZE];
> + disk_super = (struct btrfs_super_block *)buf;
> +
> + /* Already zeroed above */
> + if (btrfs_sb_offset(i) < block_count)
> + continue;
> +
> + /* Beyond actual disk size */
> + if (btrfs_sb_offset(i) >= btrfs_device_size(fd, &st))
> + continue;
> +
> + /* Does not contain any stale SB */
> + if (btrfs_read_dev_super(fd, disk_super,
> + btrfs_sb_offset(i), 0))
> + continue;
> +
> + ret = zero_dev_clamped(fd, btrfs_sb_offset(i),
> + BTRFS_SUPER_INFO_SIZE,
> + btrfs_device_size(fd, &st));
> + if (ret < 0) {
> + error("failed to zero device '%s' bytenr %llu: %s",
> + file, btrfs_sb_offset(i), strerror(-ret));
> + return 1;
> + }
> + }
> + }
> +
I told you this code can be made a lot simpler, simply by modifying the
last argument passed to zero_dev_clamped. I even posted the resulting
diff which was just 3 lines changed.
I agree that it's a good idea to wipe all available superblock when we
use -b. However I don't agree with your approach - it adds a loop, it
adds a bunch of checks and makes the complexity orders of magnitude
higher than it could be. So I'm asking again - is there any inherent
benefit which I'm missing in your newly added 35 lines of code against
just passing the block device to zero_dev_clamped and letting the
existing logic take care of everything?
> *block_count_ret = block_count;
> return 0;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] btrfs-progs: wipe copies of the stale superblock beyond -b size
2018-03-26 19:25 ` Nikolay Borisov
@ 2018-03-26 23:50 ` Anand Jain
2018-03-27 6:19 ` Nikolay Borisov
0 siblings, 1 reply; 8+ messages in thread
From: Anand Jain @ 2018-03-26 23:50 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
> I told you this code can be made a lot simpler, simply by modifying the
> last argument passed to zero_dev_clamped. I even posted the resulting
> diff which was just 3 lines changed.
>
> I agree that it's a good idea to wipe all available superblock when we
> use -b. However I don't agree with your approach - it adds a loop, it
> adds a bunch of checks and makes the complexity orders of magnitude
> higher than it could be. So I'm asking again - is there any inherent
> benefit which I'm missing in your newly added 35 lines of code against
> just passing the block device to zero_dev_clamped and letting the
> existing logic take care of everything?
I had that idea for v1 as well, but I didn't do it because it would
zero bytenr_copy#2 even when there is no btrfs superblock, (which is
fine only with in block_count).
Some might view it as corrupting the usr data (for which they have
specified -b option?)? I am just discussing I don't have any usecase
to prove it though. Do you have any idea?
If you think it should be ok, I shall go ahead and zero without
checking for the btrfs superblock beyond block_count.
Thanks, Anand
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] btrfs-progs: wipe copies of the stale superblock beyond -b size
2018-03-26 23:50 ` Anand Jain
@ 2018-03-27 6:19 ` Nikolay Borisov
2018-03-27 23:27 ` Anand Jain
0 siblings, 1 reply; 8+ messages in thread
From: Nikolay Borisov @ 2018-03-27 6:19 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
On 27.03.2018 02:50, Anand Jain wrote:
>
>
>> I told you this code can be made a lot simpler, simply by modifying the
>> last argument passed to zero_dev_clamped. I even posted the resulting
>> diff which was just 3 lines changed.
>>
>> I agree that it's a good idea to wipe all available superblock when we
>> use -b. However I don't agree with your approach - it adds a loop, it
>> adds a bunch of checks and makes the complexity orders of magnitude
>> higher than it could be. So I'm asking again - is there any inherent
>> benefit which I'm missing in your newly added 35 lines of code against
>> just passing the block device to zero_dev_clamped and letting the
>> existing logic take care of everything?
>
> I had that idea for v1 as well, but I didn't do it because it would
> zero bytenr_copy#2 even when there is no btrfs superblock, (which is
> fine only with in block_count).
>
> Some might view it as corrupting the usr data (for which they have
> specified -b option?)? I am just discussing I don't have any usecase
> to prove it though. Do you have any idea?
> If you think it should be ok, I shall go ahead and zero without
> checking for the btrfs superblock beyond block_count.
Right, so the pertinent question is - is anyone expecting to do mkfs on
a volume irrespective of the options and expect to be able to recover
data prior to the mkfs? I think the answer is 'no', but let's see what
the community says.
>
> Thanks, Anand
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] btrfs-progs: wipe copies of the stale superblock beyond -b size
2018-03-27 6:19 ` Nikolay Borisov
@ 2018-03-27 23:27 ` Anand Jain
0 siblings, 0 replies; 8+ messages in thread
From: Anand Jain @ 2018-03-27 23:27 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 03/27/2018 02:19 PM, Nikolay Borisov wrote:
>
>
> On 27.03.2018 02:50, Anand Jain wrote:
>>
>>
>>> I told you this code can be made a lot simpler, simply by modifying the
>>> last argument passed to zero_dev_clamped. I even posted the resulting
>>> diff which was just 3 lines changed.
>>>
>>> I agree that it's a good idea to wipe all available superblock when we
>>> use -b. However I don't agree with your approach - it adds a loop, it
>>> adds a bunch of checks and makes the complexity orders of magnitude
>>> higher than it could be. So I'm asking again - is there any inherent
>>> benefit which I'm missing in your newly added 35 lines of code against
>>> just passing the block device to zero_dev_clamped and letting the
>>> existing logic take care of everything?
>>
>> I had that idea for v1 as well, but I didn't do it because it would
>> zero bytenr_copy#2 even when there is no btrfs superblock, (which is
>> fine only with in block_count).
>>
>> Some might view it as corrupting the usr data (for which they have
>> specified -b option?)? I am just discussing I don't have any usecase
>> to prove it though. Do you have any idea?
>> If you think it should be ok, I shall go ahead and zero without
>> checking for the btrfs superblock beyond block_count.
>
> Right, so the pertinent question is - is anyone expecting to do mkfs on
> a volume irrespective of the options and expect to be able to recover
> data prior to the mkfs? I think the answer is 'no', but let's see what
> the community says.
Writing zero even if there is no superblock is not a question of
recovering btrfs, its a question whether there was any other good
data that is intersting to the user, for which they have specified
-b option.
Asking community is a good idea, lets take a conservative approach
to zero stale superblock only if we find a valid superblock, atleast
for now, we can add cleanups later.
Thanks, Aannd
>>
>> Thanks, Anand
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] btrfs-progs: wipe copies of the stale superblock beyond -b size
@ 2018-03-30 0:09 Anand Jain
0 siblings, 0 replies; 8+ messages in thread
From: Anand Jain @ 2018-03-30 0:09 UTC (permalink / raw)
To: linux-btrfs
During the mkfs.btrfs -b <blockcount> btrfs_prepare_device() zeros all
the superblock bytenr locations only if the bytenr is below the
blockcount. The problem with this is that if the BTRFS is recreated
with a smaller size then we will leave the stale superblock in the disk
which shall confuse the recovery. As shown in the test case below.
mkfs.btrfs -qf /dev/mapper/vg-lv
mkfs.btrfs -qf -b1G /dev/mapper/vg-lv
btrfs in dump-super -a /dev/mapper/vg-lv | grep '.fsid|superblock:'
superblock: bytenr=65536, device=/dev/mapper/vg-lv
dev_item.fsid ebc67d01-7fc5-43f0-90b4-d1925002551e [match]
superblock: bytenr=67108864, device=/dev/mapper/vg-lv
dev_item.fsid ebc67d01-7fc5-43f0-90b4-d1925002551e [match]
superblock: bytenr=274877906944, device=/dev/mapper/vg-lv
dev_item.fsid b97a9206-593b-4933-a424-c6a6ee23fe7c [match]
So if we find a valid superblock zero it even if it's beyond the
blockcount.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
utils.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/utils.c b/utils.c
index e9cb3a82fda6..6a9408b06e73 100644
--- a/utils.c
+++ b/utils.c
@@ -365,6 +365,41 @@ int btrfs_prepare_device(int fd, const char *file, u64 *block_count_ret,
return 1;
}
+ /*
+ * Check for the BTRFS SB copies up until btrfs_device_size() and zero
+ * it. So that kernel (or user for the manual recovery) don't have to
+ * confuse with the stale SB copy during recovery.
+ */
+ if (block_count != btrfs_device_size(fd, &st)) {
+ for (i = 1; i < BTRFS_SUPER_MIRROR_MAX; i++) {
+ struct btrfs_super_block *disk_super;
+ char buf[BTRFS_SUPER_INFO_SIZE];
+ disk_super = (struct btrfs_super_block *)buf;
+
+ /* Already zeroed above */
+ if (btrfs_sb_offset(i) < block_count)
+ continue;
+
+ /* Beyond actual disk size */
+ if (btrfs_sb_offset(i) >= btrfs_device_size(fd, &st))
+ continue;
+
+ /* Does not contain any stale SB */
+ if (btrfs_read_dev_super(fd, disk_super,
+ btrfs_sb_offset(i), 0))
+ continue;
+
+ ret = zero_dev_clamped(fd, btrfs_sb_offset(i),
+ BTRFS_SUPER_INFO_SIZE,
+ btrfs_device_size(fd, &st));
+ if (ret < 0) {
+ error("failed to zero device '%s' bytenr %llu: %s",
+ file, btrfs_sb_offset(i), strerror(-ret));
+ return 1;
+ }
+ }
+ }
+
*block_count_ret = block_count;
return 0;
}
--
2.7.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] btrfs-progs: wipe copies of the stale superblock beyond -b size
@ 2018-03-29 10:37 Anand Jain
0 siblings, 0 replies; 8+ messages in thread
From: Anand Jain @ 2018-03-29 10:37 UTC (permalink / raw)
To: linux-btrfs
During the mkfs.btrfs -b <blockcount> btrfs_prepare_device() zeros all
the superblock bytenr locations only if the bytenr is below the
blockcount. The problem with this is that if the BTRFS is recreated
with a smaller size then we will leave the stale superblock in the disk
which shall confuse the recovery. As shown in the test case below.
mkfs.btrfs -qf /dev/mapper/vg-lv
mkfs.btrfs -qf -b1G /dev/mapper/vg-lv
btrfs in dump-super -a /dev/mapper/vg-lv | grep '.fsid|superblock:'
superblock: bytenr=65536, device=/dev/mapper/vg-lv
dev_item.fsid ebc67d01-7fc5-43f0-90b4-d1925002551e [match]
superblock: bytenr=67108864, device=/dev/mapper/vg-lv
dev_item.fsid ebc67d01-7fc5-43f0-90b4-d1925002551e [match]
superblock: bytenr=274877906944, device=/dev/mapper/vg-lv
dev_item.fsid b97a9206-593b-4933-a424-c6a6ee23fe7c [match]
So if we find a valid superblock zero it even if it's beyond the
blockcount.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
utils.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/utils.c b/utils.c
index e9cb3a82fda6..6a9408b06e73 100644
--- a/utils.c
+++ b/utils.c
@@ -365,6 +365,41 @@ int btrfs_prepare_device(int fd, const char *file, u64 *block_count_ret,
return 1;
}
+ /*
+ * Check for the BTRFS SB copies up until btrfs_device_size() and zero
+ * it. So that kernel (or user for the manual recovery) don't have to
+ * confuse with the stale SB copy during recovery.
+ */
+ if (block_count != btrfs_device_size(fd, &st)) {
+ for (i = 1; i < BTRFS_SUPER_MIRROR_MAX; i++) {
+ struct btrfs_super_block *disk_super;
+ char buf[BTRFS_SUPER_INFO_SIZE];
+ disk_super = (struct btrfs_super_block *)buf;
+
+ /* Already zeroed above */
+ if (btrfs_sb_offset(i) < block_count)
+ continue;
+
+ /* Beyond actual disk size */
+ if (btrfs_sb_offset(i) >= btrfs_device_size(fd, &st))
+ continue;
+
+ /* Does not contain any stale SB */
+ if (btrfs_read_dev_super(fd, disk_super,
+ btrfs_sb_offset(i), 0))
+ continue;
+
+ ret = zero_dev_clamped(fd, btrfs_sb_offset(i),
+ BTRFS_SUPER_INFO_SIZE,
+ btrfs_device_size(fd, &st));
+ if (ret < 0) {
+ error("failed to zero device '%s' bytenr %llu: %s",
+ file, btrfs_sb_offset(i), strerror(-ret));
+ return 1;
+ }
+ }
+ }
+
*block_count_ret = block_count;
return 0;
}
--
2.7.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] btrfs-progs: wipe copies of the stale superblock beyond -b size
@ 2018-03-27 23:40 Anand Jain
0 siblings, 0 replies; 8+ messages in thread
From: Anand Jain @ 2018-03-27 23:40 UTC (permalink / raw)
To: linux-btrfs
During the mkfs.btrfs -b <blockcount> btrfs_prepare_device() zeros all
the superblock bytenr locations only if the bytenr is below the
blockcount. The problem with this is that if the BTRFS is recreated
with a smaller size then we will leave the stale superblock in the disk
which shall confuse the recovery. As shown in the test case below.
mkfs.btrfs -qf /dev/mapper/vg-lv
mkfs.btrfs -qf -b1G /dev/mapper/vg-lv
btrfs in dump-super -a /dev/mapper/vg-lv | grep '.fsid|superblock:'
superblock: bytenr=65536, device=/dev/mapper/vg-lv
dev_item.fsid ebc67d01-7fc5-43f0-90b4-d1925002551e [match]
superblock: bytenr=67108864, device=/dev/mapper/vg-lv
dev_item.fsid ebc67d01-7fc5-43f0-90b4-d1925002551e [match]
superblock: bytenr=274877906944, device=/dev/mapper/vg-lv
dev_item.fsid b97a9206-593b-4933-a424-c6a6ee23fe7c [match]
So if we find a valid superblock zero it even if it's beyond the
blockcount.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
utils.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/utils.c b/utils.c
index e9cb3a82fda6..6a9408b06e73 100644
--- a/utils.c
+++ b/utils.c
@@ -365,6 +365,41 @@ int btrfs_prepare_device(int fd, const char *file, u64 *block_count_ret,
return 1;
}
+ /*
+ * Check for the BTRFS SB copies up until btrfs_device_size() and zero
+ * it. So that kernel (or user for the manual recovery) don't have to
+ * confuse with the stale SB copy during recovery.
+ */
+ if (block_count != btrfs_device_size(fd, &st)) {
+ for (i = 1; i < BTRFS_SUPER_MIRROR_MAX; i++) {
+ struct btrfs_super_block *disk_super;
+ char buf[BTRFS_SUPER_INFO_SIZE];
+ disk_super = (struct btrfs_super_block *)buf;
+
+ /* Already zeroed above */
+ if (btrfs_sb_offset(i) < block_count)
+ continue;
+
+ /* Beyond actual disk size */
+ if (btrfs_sb_offset(i) >= btrfs_device_size(fd, &st))
+ continue;
+
+ /* Does not contain any stale SB */
+ if (btrfs_read_dev_super(fd, disk_super,
+ btrfs_sb_offset(i), 0))
+ continue;
+
+ ret = zero_dev_clamped(fd, btrfs_sb_offset(i),
+ BTRFS_SUPER_INFO_SIZE,
+ btrfs_device_size(fd, &st));
+ if (ret < 0) {
+ error("failed to zero device '%s' bytenr %llu: %s",
+ file, btrfs_sb_offset(i), strerror(-ret));
+ return 1;
+ }
+ }
+ }
+
*block_count_ret = block_count;
return 0;
}
--
2.7.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-03-30 0:12 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-26 8:27 [PATCH] btrfs-progs: wipe copies of the stale superblock beyond -b size Anand Jain
2018-03-26 19:25 ` Nikolay Borisov
2018-03-26 23:50 ` Anand Jain
2018-03-27 6:19 ` Nikolay Borisov
2018-03-27 23:27 ` Anand Jain
2018-03-27 23:40 Anand Jain
2018-03-29 10:37 Anand Jain
2018-03-30 0:09 Anand Jain
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.