| Message ID | 7AF66DC0FE0640D1A4E007C4DAC16AC4@alyakaslap (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2016-05-17 07:24, Alex Lyakas wrote: > RFC: This patch not for merging, but only for review and discussion. > > When mounting, we consider only the primary superblock on each device. > But when writing the superblocks, we might silently ignore errors > from the primary superblock, if we succeeded to write secondary > superblocks. In such case, the primary superblock was not updated > properly, and if we crash at this point, later mount will use > an out-of-date superblock. > > This patch changes the behavior to NOT IGNORING any errors on the > primary superblock, > and IGNORING any errors on secondary superblocks. This way, we always > insist on having > an up-to-date primary superblock. I don't entirely agree with this reasoning. We absolutely should not be ignoring errors when writing to the primary superblock, but there is no reason we should be ignoring them when writing backup superblocks either. Ideally, all superblocks should be up-to-date and consistent with each other, as we can't be certain which (if any) of them will be readable without errors the next time we mount the filesystem. We can't really provide this consistency guarantee though, because of how modern storage devices work (we can't atomically update all three superblocks at the same time), but that doesn't mean we shouldn't retry writing a backup superblock if there's an error doing so the first time. > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 4e47849..0ae9f7c 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -3357,11 +3357,13 @@ static int write_dev_supers(struct btrfs_device > *device, > bh = __find_get_block(device->bdev, bytenr / 4096, > BTRFS_SUPER_INFO_SIZE); > if (!bh) { > - errors++; > + /* we only care about primary superblock errors */ > + if (i == 0) > + errors++; > continue; > } > wait_on_buffer(bh); > - if (!buffer_uptodate(bh)) > + if (!buffer_uptodate(bh) && i == 0) > errors++; > > /* drop our reference */ > @@ -3388,9 +3390,10 @@ static int write_dev_supers(struct btrfs_device > *device, > BTRFS_SUPER_INFO_SIZE); > if (!bh) { > btrfs_err(device->dev_root->fs_info, > - "couldn't get super buffer head for bytenr %llu", > - bytenr); > - errors++; > + "couldn't get super buffer head for bytenr %llu (sb > copy %d)", > + bytenr, i); > + if (i == 0) > + errors++; > continue; > } > > @@ -3413,10 +3416,10 @@ static int write_dev_supers(struct btrfs_device > *device, > ret = btrfsic_submit_bh(WRITE_FUA, bh); > else > ret = btrfsic_submit_bh(WRITE_SYNC, bh); > - if (ret) > + if (ret && i == 0) > errors++; > } > - return errors < i ? 0 : -1; > + return errors ? -1 : 0; > } > > /* > > > P.S.: when reviewing the code of write_dev_supers(), I also noticed that > when wait==0 and we hit an error in one __getblk(), then the caller > (write_all_supers) will not properly wait for submitted buffer-heads to > complete, and we won't do the additional "brelse(bh);", which wait==0 > case does. Is this a problem? -- 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 4e47849..0ae9f7c 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -3357,11 +3357,13 @@ static int write_dev_supers(struct btrfs_device *device, bh = __find_get_block(device->bdev, bytenr / 4096, BTRFS_SUPER_INFO_SIZE); if (!bh) { - errors++; + /* we only care about primary superblock errors */ + if (i == 0) + errors++; continue; } wait_on_buffer(bh); - if (!buffer_uptodate(bh)) + if (!buffer_uptodate(bh) && i == 0) errors++;
RFC: This patch not for merging, but only for review and discussion. When mounting, we consider only the primary superblock on each device. But when writing the superblocks, we might silently ignore errors from the primary superblock, if we succeeded to write secondary superblocks. In such case, the primary superblock was not updated properly, and if we crash at this point, later mount will use an out-of-date superblock. This patch changes the behavior to NOT IGNORING any errors on the primary superblock, and IGNORING any errors on secondary superblocks. This way, we always insist on having an up-to-date primary superblock. /* drop our reference */ @@ -3388,9 +3390,10 @@ static int write_dev_supers(struct btrfs_device *device, BTRFS_SUPER_INFO_SIZE); if (!bh) { btrfs_err(device->dev_root->fs_info, - "couldn't get super buffer head for bytenr %llu", - bytenr); - errors++; + "couldn't get super buffer head for bytenr %llu (sb copy %d)", + bytenr, i); + if (i == 0) + errors++; continue; } @@ -3413,10 +3416,10 @@ static int write_dev_supers(struct btrfs_device *device, ret = btrfsic_submit_bh(WRITE_FUA, bh); else ret = btrfsic_submit_bh(WRITE_SYNC, bh); - if (ret) + if (ret && i == 0) errors++; } - return errors < i ? 0 : -1; + return errors ? -1 : 0; } /* P.S.: when reviewing the code of write_dev_supers(), I also noticed that when wait==0 and we hit an error in one __getblk(), then the caller (write_all_supers) will not properly wait for submitted buffer-heads to complete, and we won't do the additional "brelse(bh);", which wait==0 case does. Is this a problem? -- 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