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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
