| Message ID | 1512119984-12708-6-git-send-email-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/01/2017 05:19 PM, Nikolay Borisov wrote: > Currently this function executes the inner loop at most 1 due to the i = 0; > i < 1 condition. Furthermore, the btrfs_super_generation(super) > transid code > in the if condition is never executed due to latest always set to NULL hence the > first part of the condition always triggering. The gist of btrfs_read_dev_super > is really to read the first superblock. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/disk-io.c | 27 ++++----------------------- > 1 file changed, 4 insertions(+), 23 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 82c96607fc46..6d5f632fd1e7 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -3170,37 +3170,18 @@ int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num, > struct buffer_head *btrfs_read_dev_super(struct block_device *bdev) > { > struct buffer_head *bh; > - struct buffer_head *latest = NULL; > - struct btrfs_super_block *super; > - int i; > - u64 transid = 0; > - int ret = -EINVAL; > + int ret; > > /* we would like to check all the supers, but that would make > * a btrfs mount succeed after a mkfs from a different FS. > * So, we need to add a special mount option to scan for > * later supers, using BTRFS_SUPER_MIRROR_MAX instead > */ We need below loop to support the above comment at some point, instead of removing I would prefer to fix as per above comments. Thanks, Anand > - for (i = 0; i < 1; i++) { > - ret = btrfs_read_dev_one_super(bdev, i, &bh); > - if (ret) > - continue; > - > - super = (struct btrfs_super_block *)bh->b_data; > - > - if (!latest || btrfs_super_generation(super) > transid) { > - brelse(latest); > - latest = bh; > - transid = btrfs_super_generation(super); > - } else { > - brelse(bh); > - } > - } > - > - if (!latest) > + ret = btrfs_read_dev_one_super(bdev, 0, &bh); > + if (ret) > return ERR_PTR(ret); > > - return latest; > + return bh; > } > > /* > -- 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 2.12.2017 01:23, Anand Jain wrote: > > > On 12/01/2017 05:19 PM, Nikolay Borisov wrote: >> Currently this function executes the inner loop at most 1 due to the i >> = 0; >> i < 1 condition. Furthermore, the btrfs_super_generation(super) > >> transid code >> in the if condition is never executed due to latest always set to NULL >> hence the >> first part of the condition always triggering. The gist of >> btrfs_read_dev_super >> is really to read the first superblock. >> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com> >> --- >> fs/btrfs/disk-io.c | 27 ++++----------------------- >> 1 file changed, 4 insertions(+), 23 deletions(-) >> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >> index 82c96607fc46..6d5f632fd1e7 100644 >> --- a/fs/btrfs/disk-io.c >> +++ b/fs/btrfs/disk-io.c >> @@ -3170,37 +3170,18 @@ int btrfs_read_dev_one_super(struct >> block_device *bdev, int copy_num, >> struct buffer_head *btrfs_read_dev_super(struct block_device *bdev) >> { >> struct buffer_head *bh; >> - struct buffer_head *latest = NULL; >> - struct btrfs_super_block *super; >> - int i; >> - u64 transid = 0; >> - int ret = -EINVAL; >> + int ret; >> /* we would like to check all the supers, but that would make >> * a btrfs mount succeed after a mkfs from a different FS. >> * So, we need to add a special mount option to scan for >> * later supers, using BTRFS_SUPER_MIRROR_MAX instead >> */ > > We need below loop to support the above comment at some point, And when is that, since I don't see anyone working on it. Furthermore what is it that we are losing in terms of functionality by not supporting the comment? It seems this code was just slapt here without any vision how/when to implement it? Furthermore, you seem to be aware of what the comment is talking about, I have to admit I'm not. Is the idea that if another filesystem does mkfs and doesn't overwrite ALL superblock copies that btrfs writes (at 64k, 64mb, 256gb and 1 PiB) then it's possible for this code to erroneously detect btrfs when in fact there is a different fs? I don't understand what problem *should* be solved here... > instead of removing I would prefer to fix as per above comments. > > Thanks, Anand > > >> - for (i = 0; i < 1; i++) { >> - ret = btrfs_read_dev_one_super(bdev, i, &bh); >> - if (ret) >> - continue; >> - >> - super = (struct btrfs_super_block *)bh->b_data; >> - >> - if (!latest || btrfs_super_generation(super) > transid) { >> - brelse(latest); >> - latest = bh; >> - transid = btrfs_super_generation(super); >> - } else { >> - brelse(bh); >> - } >> - } >> - >> - if (!latest) >> + ret = btrfs_read_dev_one_super(bdev, 0, &bh); >> + if (ret) >> return ERR_PTR(ret); >> - return latest; >> + return bh; >> } >> /* >> > -- 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 12/03/2017 05:43 PM, Nikolay Borisov wrote: > > > On 2.12.2017 01:23, Anand Jain wrote: >> >> >> On 12/01/2017 05:19 PM, Nikolay Borisov wrote: >>> Currently this function executes the inner loop at most 1 due to the i >>> = 0; >>> i < 1 condition. Furthermore, the btrfs_super_generation(super) > >>> transid code >>> in the if condition is never executed due to latest always set to NULL >>> hence the >>> first part of the condition always triggering. The gist of >>> btrfs_read_dev_super >>> is really to read the first superblock. >>> >>> Signed-off-by: Nikolay Borisov <nborisov@suse.com> >>> --- >>> fs/btrfs/disk-io.c | 27 ++++----------------------- >>> 1 file changed, 4 insertions(+), 23 deletions(-) >>> >>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >>> index 82c96607fc46..6d5f632fd1e7 100644 >>> --- a/fs/btrfs/disk-io.c >>> +++ b/fs/btrfs/disk-io.c >>> @@ -3170,37 +3170,18 @@ int btrfs_read_dev_one_super(struct >>> block_device *bdev, int copy_num, >>> struct buffer_head *btrfs_read_dev_super(struct block_device *bdev) >>> { >>> struct buffer_head *bh; >>> - struct buffer_head *latest = NULL; >>> - struct btrfs_super_block *super; >>> - int i; >>> - u64 transid = 0; >>> - int ret = -EINVAL; >>> + int ret; >>> /* we would like to check all the supers, but that would make >>> * a btrfs mount succeed after a mkfs from a different FS. >>> * So, we need to add a special mount option to scan for >>> * later supers, using BTRFS_SUPER_MIRROR_MAX instead >>> */ >> >> We need below loop to support the above comment at some point, > > And when is that, since I don't see anyone working on it. > Furthermore > what is it that we are losing in terms of functionality by not > supporting the comment? As of now if the primary SB is corrupted we don't recover from it automatically and external tools are broken. >It seems this code was just slapt here without > any vision how/when to implement it? I have in my todo list. I am ok if you want to fix it as needed. > Furthermore, you seem to be aware of what the comment is talking about, > I have to admit I'm not. > Is the idea that if another filesystem does > mkfs and doesn't overwrite ALL superblock copies that btrfs writes (at > 64k, 64mb, 256gb and 1 PiB) then it's possible for this code to > erroneously detect btrfs when in fact there is a different fs? Right. IMO the above comment is wrong as well for which it made the for-loop to read only primary SB. If we have a feature to maintain backup SB, then that feature is only complete when we would automatically recover from the backup SB. If a user overwrites btrfs primary SB and still mounts btrfs with -t btrfs options, then its use-end problem we should be able to recover from backup SB. So IMO looping through other SB is fine. Thanks, Anand > I don't understand what problem *should* be solved here... >> instead of removing I would prefer to fix as per above comments. >> >> Thanks, Anand >> >> >>> - for (i = 0; i < 1; i++) { >>> - ret = btrfs_read_dev_one_super(bdev, i, &bh); >>> - if (ret) >>> - continue; >>> - >>> - super = (struct btrfs_super_block *)bh->b_data; >>> - >>> - if (!latest || btrfs_super_generation(super) > transid) { >>> - brelse(latest); >>> - latest = bh; >>> - transid = btrfs_super_generation(super); >>> - } else { >>> - brelse(bh); >>> - } >>> - } >>> - >>> - if (!latest) >>> + ret = btrfs_read_dev_one_super(bdev, 0, &bh); >>> + if (ret) >>> return ERR_PTR(ret); >>> - return latest; >>> + return bh; >>> } >>> /* >>> >> > -- > 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 Sun, Dec 03, 2017 at 11:43:13AM +0200, Nikolay Borisov wrote: > On 2.12.2017 01:23, Anand Jain wrote: > > On 12/01/2017 05:19 PM, Nikolay Borisov wrote: > >> Currently this function executes the inner loop at most 1 due to the i > >> = 0; > >> i < 1 condition. Furthermore, the btrfs_super_generation(super) > > >> transid code > >> in the if condition is never executed due to latest always set to NULL > >> hence the > >> first part of the condition always triggering. The gist of > >> btrfs_read_dev_super > >> is really to read the first superblock. > >> > >> Signed-off-by: Nikolay Borisov <nborisov@suse.com> > >> --- > >> fs/btrfs/disk-io.c | 27 ++++----------------------- > >> 1 file changed, 4 insertions(+), 23 deletions(-) > >> > >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > >> index 82c96607fc46..6d5f632fd1e7 100644 > >> --- a/fs/btrfs/disk-io.c > >> +++ b/fs/btrfs/disk-io.c > >> @@ -3170,37 +3170,18 @@ int btrfs_read_dev_one_super(struct > >> block_device *bdev, int copy_num, > >> struct buffer_head *btrfs_read_dev_super(struct block_device *bdev) > >> { > >> struct buffer_head *bh; > >> - struct buffer_head *latest = NULL; > >> - struct btrfs_super_block *super; > >> - int i; > >> - u64 transid = 0; > >> - int ret = -EINVAL; > >> + int ret; > >> /* we would like to check all the supers, but that would make > >> * a btrfs mount succeed after a mkfs from a different FS. > >> * So, we need to add a special mount option to scan for > >> * later supers, using BTRFS_SUPER_MIRROR_MAX instead > >> */ > > > > We need below loop to support the above comment at some point, > > And when is that, since I don't see anyone working on it. Furthermore > what is it that we are losing in terms of functionality by not > supporting the comment? It seems this code was just slapt here without > any vision how/when to implement it? > > Furthermore, you seem to be aware of what the comment is talking about, > I have to admit I'm not. Is the idea that if another filesystem does > mkfs and doesn't overwrite ALL superblock copies that btrfs writes (at > 64k, 64mb, 256gb and 1 PiB) then it's possible for this code to > erroneously detect btrfs when in fact there is a different fs? > > I don't understand what problem *should* be solved here... Without any further checks and validation, we cannot simply iterate over all superblocks and try to mount anything that looks ok. Even if the offsets exist on the block device. The fsid should be at least checked, but that's still not enough to ensure the 1st copy is what we want to mount. In that case it's up to the user to deal with the problem vs automating all fallbacks in the kernel by default. We could enhance the recovery options (usebackuproot) to look at the other superblocks. An example scenario: - mkfs, contains 2 superblocks (primary and 1st copy) - shrink the filesystem, 1st copy contains stale version of the same filesystem - mount, primary is broken, use 1st backup copy -- now, this could actually succeed if all metadata pointed by the 1st copy exist and are consistent and we'd end up with an older version of the filesystem metadata, and partially data Another example, and this was probably meant by the comment: - mkfs filesystem, contains primary and 1st (or more) superblock copy - mkfs again, now contains fewer sb copies - mount, similar situation as above, though the fsid will mismatch (provided we're able to read the primary sb at all) The commit introducing the comment is from 2008, nobody saw the need to add the validation. And from the usability POV, the current behaviour looks better because the user/admin has a chance to analyze the problem first, run fsck before mounting from the sb copies and then use 'btrfs-select-super' to overwrite the primary sb with the backup copy. If there are clear guarantees and enough validation for the sb copy use, then fine, otherwise we can apply this patch and extend the comment so it's documented why we don't iterate. -- 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 4.12.2017 16:13, David Sterba wrote: > On Sun, Dec 03, 2017 at 11:43:13AM +0200, Nikolay Borisov wrote: >> On 2.12.2017 01:23, Anand Jain wrote: >>> On 12/01/2017 05:19 PM, Nikolay Borisov wrote: >>>> Currently this function executes the inner loop at most 1 due to the i >>>> = 0; >>>> i < 1 condition. Furthermore, the btrfs_super_generation(super) > >>>> transid code >>>> in the if condition is never executed due to latest always set to NULL >>>> hence the >>>> first part of the condition always triggering. The gist of >>>> btrfs_read_dev_super >>>> is really to read the first superblock. >>>> >>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com> >>>> --- >>>> fs/btrfs/disk-io.c | 27 ++++----------------------- >>>> 1 file changed, 4 insertions(+), 23 deletions(-) >>>> >>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >>>> index 82c96607fc46..6d5f632fd1e7 100644 >>>> --- a/fs/btrfs/disk-io.c >>>> +++ b/fs/btrfs/disk-io.c >>>> @@ -3170,37 +3170,18 @@ int btrfs_read_dev_one_super(struct >>>> block_device *bdev, int copy_num, >>>> struct buffer_head *btrfs_read_dev_super(struct block_device *bdev) >>>> { >>>> struct buffer_head *bh; >>>> - struct buffer_head *latest = NULL; >>>> - struct btrfs_super_block *super; >>>> - int i; >>>> - u64 transid = 0; >>>> - int ret = -EINVAL; >>>> + int ret; >>>> /* we would like to check all the supers, but that would make >>>> * a btrfs mount succeed after a mkfs from a different FS. >>>> * So, we need to add a special mount option to scan for >>>> * later supers, using BTRFS_SUPER_MIRROR_MAX instead >>>> */ >>> >>> We need below loop to support the above comment at some point, >> >> And when is that, since I don't see anyone working on it. Furthermore >> what is it that we are losing in terms of functionality by not >> supporting the comment? It seems this code was just slapt here without >> any vision how/when to implement it? >> >> Furthermore, you seem to be aware of what the comment is talking about, >> I have to admit I'm not. Is the idea that if another filesystem does >> mkfs and doesn't overwrite ALL superblock copies that btrfs writes (at >> 64k, 64mb, 256gb and 1 PiB) then it's possible for this code to >> erroneously detect btrfs when in fact there is a different fs? >> >> I don't understand what problem *should* be solved here... > > Without any further checks and validation, we cannot simply iterate over > all superblocks and try to mount anything that looks ok. Even if the > offsets exist on the block device. The fsid should be at least checked, > but that's still not enough to ensure the 1st copy is what we want to > mount. I'm more inclined to agree with Anand here, that if the users wants to mount with -t btrfs then the filesystem should assume it's correct to use any of the additional superblocks. Otherwise we should explicitly state somewhere that the superblock copies are there only for the sake of the user-space tools, in which case this patch can go in, albeit with some rewording. > > In that case it's up to the user to deal with the problem vs automating > all fallbacks in the kernel by default. We could enhance the recovery > options (usebackuproot) to look at the other superblocks. > > An example scenario: > > - mkfs, contains 2 superblocks (primary and 1st copy) > - shrink the filesystem, 1st copy contains stale version of the same > filesystem > - mount, primary is broken, use 1st backup copy -- now, this could > actually succeed if all metadata pointed by the 1st copy exist and are > consistent and we'd end up with an older version of the filesystem > metadata, and partially data > > > Another example, and this was probably meant by the comment: > > - mkfs filesystem, contains primary and 1st (or more) superblock copy > - mkfs again, now contains fewer sb copies > - mount, similar situation as above, though the fsid will mismatch > (provided we're able to read the primary sb at all) > > > The commit introducing the comment is from 2008, nobody saw the need to > add the validation. And from the usability POV, the current behaviour > looks better because the user/admin has a chance to analyze the problem > first, run fsck before mounting from the sb copies and then use > 'btrfs-select-super' to overwrite the primary sb with the backup copy. > > If there are clear guarantees and enough validation for the sb copy use, > then fine, otherwise we can apply this patch and extend the comment so > it's documented why we don't iterate. > -- 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 Mon, Dec 04, 2017 at 06:20:09PM +0200, Nikolay Borisov wrote: > >> I don't understand what problem *should* be solved here... > > > > Without any further checks and validation, we cannot simply iterate over > > all superblocks and try to mount anything that looks ok. Even if the > > offsets exist on the block device. The fsid should be at least checked, > > but that's still not enough to ensure the 1st copy is what we want to > > mount. > > I'm more inclined to agree with Anand here, that if the users wants to > mount with -t btrfs then the filesystem should assume it's correct to > use any of the additional superblocks. If and only if the additional superblocks are valid. And if we can't read the primary superblock, we don't have enough information to establish the validation. EIO? We don't have anything. CRC mismatch? Can't trust the whole data. We need FSID and total_size at least. Other actions are limited from inside kernel. > Otherwise we should explicitly > state somewhere that the superblock copies are there only for the sake > of the user-space tools, in which case this patch can go in, albeit with > some rewording. Unless there's something I'm missing to address the concerns above, I'm for (an updated version) of your 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
On 12/07/2017 11:07 PM, Anand Jain wrote: > > > On 12/07/2017 12:24 AM, David Sterba wrote: >> On Mon, Dec 04, 2017 at 06:20:09PM +0200, Nikolay Borisov wrote: >>>>> I don't understand what problem *should* be solved here... >>>> >>>> Without any further checks and validation, we cannot simply iterate >>>> over >>>> all superblocks and try to mount anything that looks ok. Even if the >>>> offsets exist on the block device. The fsid should be at least >>>> checked, >>>> but that's still not enough to ensure the 1st copy is what we want to >>>> mount. >>> >>> I'm more inclined to agree with Anand here, that if the users wants to >>> mount with -t btrfs then the filesystem should assume it's correct to >>> use any of the additional superblocks. >> >> If and only if the additional superblocks are valid. And if we can't >> read the primary superblock, we don't have enough information to >> establish the validation. EIO? We don't have anything. CRC mismatch? >> Can't trust the whole data. We need FSID and total_size at least. Other >> actions are limited from inside kernel. ext4 has mount option -o sb=<copy_num> to specify the copy num to use. Not sure if I have dug enough but as of now I don't find any pitfall. Only funny thing is you can mount a device even after wipefs -a and recover its primary SB, to which my view is that the problem is at the wipefs -a end, not able to wipe all SB copies. I sent out an experimental RFC patch here: [PATCH RFC] btrfs: self heal from SB fail Pls try out, any problem that you could think off by this approach. Thanks, Anand -- 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/disk-io.c b/fs/btrfs/disk-io.c index 82c96607fc46..6d5f632fd1e7 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -3170,37 +3170,18 @@ int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num, struct buffer_head *btrfs_read_dev_super(struct block_device *bdev) { struct buffer_head *bh; - struct buffer_head *latest = NULL; - struct btrfs_super_block *super; - int i; - u64 transid = 0; - int ret = -EINVAL; + int ret; /* we would like to check all the supers, but that would make * a btrfs mount succeed after a mkfs from a different FS. * So, we need to add a special mount option to scan for * later supers, using BTRFS_SUPER_MIRROR_MAX instead */ - for (i = 0; i < 1; i++) { - ret = btrfs_read_dev_one_super(bdev, i, &bh); - if (ret) - continue; - - super = (struct btrfs_super_block *)bh->b_data; - - if (!latest || btrfs_super_generation(super) > transid) { - brelse(latest); - latest = bh; - transid = btrfs_super_generation(super); - } else { - brelse(bh); - } - } - - if (!latest) + ret = btrfs_read_dev_one_super(bdev, 0, &bh); + if (ret) return ERR_PTR(ret); - return latest; + return bh; } /*
Currently this function executes the inner loop at most 1 due to the i = 0; i < 1 condition. Furthermore, the btrfs_super_generation(super) > transid code in the if condition is never executed due to latest always set to NULL hence the first part of the condition always triggering. The gist of btrfs_read_dev_super is really to read the first superblock. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/disk-io.c | 27 ++++----------------------- 1 file changed, 4 insertions(+), 23 deletions(-)