| Message ID | 1343821552-28726-1-git-send-email-sbehrens@giantdisaster.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/01/2012 07:45 PM, Stefan Behrens wrote: > With commit acce952b0, btrfs was changed to flag the filesystem with > BTRFS_SUPER_FLAG_ERROR and switch to read-only mode after a fatal > error happened like a write I/O errors of all mirrors. > In such situations, on unmount, the superblock is written in > btrfs_error_commit_super(). This is done with the intention to be able > to evaluate the error flag on the next mount. A warning is printed > in this case during the next mount and the log tree is ignored. > > The issue is that it is possible that the superblock points to a root > that was not written (due to write I/O errors). > The result is that the filesystem cannot be mounted. btrfsck also does > not start and all the other btrfs-progs tools fail to start as well. > However, mount -o recovery is working well and does the right things > to recover the filesystem (i.e., don't use the log root, clear the > free space cache and use the next mountable root that is stored in the > root backup array). > > This patch removes the writing of the superblock when > BTRFS_SUPER_FLAG_ERROR is set, and removes the handling of the error > flag in the mount function. > Yes, I have to admit that this can be a serious problem. But we'll need to send the error flag stored in the super block into disk in the future so that the next mount can find it unstable and do fsck by itself maybe. If we do not write the super any more, it cannot find the flag. So can we find a way to make both things happy? thanks, liubo > These lines can be used to reproduce the issue (using /dev/sdm): > SCRATCH_DEV=/dev/sdm > SCRATCH_MNT=/mnt > echo 0 25165824 linear $SCRATCH_DEV 0 | dmsetup create foo > ls -alLF /dev/mapper/foo > mkfs.btrfs /dev/mapper/foo > mount /dev/mapper/foo $SCRATCH_MNT > echo bar > $SCRATCH_MNT/foo > sync > echo 0 25165824 error | dmsetup reload foo > dmsetup resume foo > ls -alF $SCRATCH_MNT > touch $SCRATCH_MNT/1 > ls -alF $SCRATCH_MNT > sleep 35 > echo 0 25165824 linear $SCRATCH_DEV 0 | dmsetup reload foo > dmsetup resume foo > sleep 1 > umount $SCRATCH_MNT > btrfsck /dev/mapper/foo > dmsetup remove foo > > Signed-off-by: Stefan Behrens <sbehrens@giantdisaster.de> > Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net> > --- > Changes v1 -> v2: > - Removed one large comment entirely which was not needed anymore > after the code changes. > - Removed a warning message which was dead code since the super block > is not written anymore when the flag BTRFS_SUPER_FLAG_ERROR is set. > - Removed the return code of btrfs_error_commit_super() since it now > does not return any errors anymore. > > fs/btrfs/disk-io.c | 36 ++++-------------------------------- > fs/btrfs/disk-io.h | 2 +- > 2 files changed, 5 insertions(+), 33 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 502b20c..f6a1d0f 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -2531,8 +2531,7 @@ retry_root_backup: > goto fail_trans_kthread; > > /* do not make disk changes in broken FS */ > - if (btrfs_super_log_root(disk_super) != 0 && > - !(fs_info->fs_state & BTRFS_SUPER_FLAG_ERROR)) { > + if (btrfs_super_log_root(disk_super) != 0) { > u64 bytenr = btrfs_super_log_root(disk_super); > > if (fs_devices->rw_devices == 0) { > @@ -3192,30 +3191,14 @@ int close_ctree(struct btrfs_root *root) > /* clear out the rbtree of defraggable inodes */ > btrfs_run_defrag_inodes(fs_info); > > - /* > - * Here come 2 situations when btrfs is broken to flip readonly: > - * > - * 1. when btrfs flips readonly somewhere else before > - * btrfs_commit_super, sb->s_flags has MS_RDONLY flag, > - * and btrfs will skip to write sb directly to keep > - * ERROR state on disk. > - * > - * 2. when btrfs flips readonly just in btrfs_commit_super, > - * and in such case, btrfs cannot write sb via btrfs_commit_super, > - * and since fs_state has been set BTRFS_SUPER_FLAG_ERROR flag, > - * btrfs will cleanup all FS resources first and write sb then. > - */ > if (!(fs_info->sb->s_flags & MS_RDONLY)) { > ret = btrfs_commit_super(root); > if (ret) > printk(KERN_ERR "btrfs: commit super ret %d\n", ret); > } > > - if (fs_info->fs_state & BTRFS_SUPER_FLAG_ERROR) { > - ret = btrfs_error_commit_super(root); > - if (ret) > - printk(KERN_ERR "btrfs: commit super ret %d\n", ret); > - } > + if (fs_info->fs_state & BTRFS_SUPER_FLAG_ERROR) > + btrfs_error_commit_super(root); > > btrfs_put_block_group_cache(fs_info); > > @@ -3437,18 +3420,11 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, > if (read_only) > return 0; > > - if (fs_info->fs_state & BTRFS_SUPER_FLAG_ERROR) { > - printk(KERN_WARNING "warning: mount fs with errors, " > - "running btrfsck is recommended\n"); > - } > - > return 0; > } > > -int btrfs_error_commit_super(struct btrfs_root *root) > +void btrfs_error_commit_super(struct btrfs_root *root) > { > - int ret; > - > mutex_lock(&root->fs_info->cleaner_mutex); > btrfs_run_delayed_iputs(root); > mutex_unlock(&root->fs_info->cleaner_mutex); > @@ -3458,10 +3434,6 @@ int btrfs_error_commit_super(struct btrfs_root *root) > > /* cleanup FS via transaction */ > btrfs_cleanup_transaction(root); > - > - ret = write_ctree_super(NULL, root, 0); > - > - return ret; > } > > static void btrfs_destroy_ordered_operations(struct btrfs_root *root) > diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h > index 95e147e..c5b00a7 100644 > --- a/fs/btrfs/disk-io.h > +++ b/fs/btrfs/disk-io.h > @@ -54,7 +54,7 @@ int write_ctree_super(struct btrfs_trans_handle *trans, > struct btrfs_root *root, int max_mirrors); > struct buffer_head *btrfs_read_dev_super(struct block_device *bdev); > int btrfs_commit_super(struct btrfs_root *root); > -int btrfs_error_commit_super(struct btrfs_root *root); > +void btrfs_error_commit_super(struct btrfs_root *root); > struct extent_buffer *btrfs_find_tree_block(struct btrfs_root *root, > u64 bytenr, u32 blocksize); > struct btrfs_root *btrfs_read_fs_root_no_radix(struct btrfs_root *tree_root, > -- 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 Wed, August 01, 2012 at 14:02 (+0200), Liu Bo wrote: > On 08/01/2012 07:45 PM, Stefan Behrens wrote: >> With commit acce952b0, btrfs was changed to flag the filesystem with >> BTRFS_SUPER_FLAG_ERROR and switch to read-only mode after a fatal >> error happened like a write I/O errors of all mirrors. >> In such situations, on unmount, the superblock is written in >> btrfs_error_commit_super(). This is done with the intention to be able >> to evaluate the error flag on the next mount. A warning is printed >> in this case during the next mount and the log tree is ignored. >> >> The issue is that it is possible that the superblock points to a root >> that was not written (due to write I/O errors). >> The result is that the filesystem cannot be mounted. btrfsck also does >> not start and all the other btrfs-progs tools fail to start as well. >> However, mount -o recovery is working well and does the right things >> to recover the filesystem (i.e., don't use the log root, clear the >> free space cache and use the next mountable root that is stored in the >> root backup array). >> >> This patch removes the writing of the superblock when >> BTRFS_SUPER_FLAG_ERROR is set, and removes the handling of the error >> flag in the mount function. >> > > Yes, I have to admit that this can be a serious problem. > > But we'll need to send the error flag stored in the super block into > disk in the future so that the next mount can find it unstable and do > fsck by itself maybe. Hum, that's possible. However, I neither see a) a safe way to get that flag to disk nor b) a situation where this flag would help. When we abort a transaction, we just roll everything back to the last commit, i.e. a consistent state. So if we stop writing a potentially corrupt super block, we should be fine anyway. Or am I missing something? -Jan -- 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 08/01/2012 09:07 PM, Jan Schmidt wrote: > On Wed, August 01, 2012 at 14:02 (+0200), Liu Bo wrote: >> On 08/01/2012 07:45 PM, Stefan Behrens wrote: >>> With commit acce952b0, btrfs was changed to flag the filesystem with >>> BTRFS_SUPER_FLAG_ERROR and switch to read-only mode after a fatal >>> error happened like a write I/O errors of all mirrors. >>> In such situations, on unmount, the superblock is written in >>> btrfs_error_commit_super(). This is done with the intention to be able >>> to evaluate the error flag on the next mount. A warning is printed >>> in this case during the next mount and the log tree is ignored. >>> >>> The issue is that it is possible that the superblock points to a root >>> that was not written (due to write I/O errors). >>> The result is that the filesystem cannot be mounted. btrfsck also does >>> not start and all the other btrfs-progs tools fail to start as well. >>> However, mount -o recovery is working well and does the right things >>> to recover the filesystem (i.e., don't use the log root, clear the >>> free space cache and use the next mountable root that is stored in the >>> root backup array). >>> >>> This patch removes the writing of the superblock when >>> BTRFS_SUPER_FLAG_ERROR is set, and removes the handling of the error >>> flag in the mount function. >>> >> >> Yes, I have to admit that this can be a serious problem. >> >> But we'll need to send the error flag stored in the super block into >> disk in the future so that the next mount can find it unstable and do >> fsck by itself maybe. > > Hum, that's possible. However, I neither see > > a) a safe way to get that flag to disk > > nor > > b) a situation where this flag would help. When we abort a transaction, we just > roll everything back to the last commit, i.e. a consistent state. So if we stop > writing a potentially corrupt super block, we should be fine anyway. Or am I > missing something? > I'm just wondering if we can roll everything back well, why do we need fsck? thanks, liubo > -Jan > -- 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 01.08.2012 15:31, Liu Bo wrote: > On 08/01/2012 09:07 PM, Jan Schmidt wrote: >> On Wed, August 01, 2012 at 14:02 (+0200), Liu Bo wrote: >>> On 08/01/2012 07:45 PM, Stefan Behrens wrote: >>>> With commit acce952b0, btrfs was changed to flag the filesystem with >>>> BTRFS_SUPER_FLAG_ERROR and switch to read-only mode after a fatal >>>> error happened like a write I/O errors of all mirrors. >>>> In such situations, on unmount, the superblock is written in >>>> btrfs_error_commit_super(). This is done with the intention to be able >>>> to evaluate the error flag on the next mount. A warning is printed >>>> in this case during the next mount and the log tree is ignored. >>>> >>>> The issue is that it is possible that the superblock points to a root >>>> that was not written (due to write I/O errors). >>>> The result is that the filesystem cannot be mounted. btrfsck also does >>>> not start and all the other btrfs-progs tools fail to start as well. >>>> However, mount -o recovery is working well and does the right things >>>> to recover the filesystem (i.e., don't use the log root, clear the >>>> free space cache and use the next mountable root that is stored in the >>>> root backup array). >>>> >>>> This patch removes the writing of the superblock when >>>> BTRFS_SUPER_FLAG_ERROR is set, and removes the handling of the error >>>> flag in the mount function. >>>> >>> >>> Yes, I have to admit that this can be a serious problem. >>> >>> But we'll need to send the error flag stored in the super block into >>> disk in the future so that the next mount can find it unstable and do >>> fsck by itself maybe. >> >> Hum, that's possible. However, I neither see >> >> a) a safe way to get that flag to disk >> >> nor >> >> b) a situation where this flag would help. When we abort a transaction, we just >> roll everything back to the last commit, i.e. a consistent state. So if we stop >> writing a potentially corrupt super block, we should be fine anyway. Or am I >> missing something? >> > > I'm just wondering if we can roll everything back well, why do we need fsck? Mostly for undetected errors. > > thanks, > liubo > >> -Jan >> > > -- > 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 Wed, 01 Aug 2012 21:31:58 +0800, Liu Bo wrote: > On 08/01/2012 09:07 PM, Jan Schmidt wrote: >> On Wed, August 01, 2012 at 14:02 (+0200), Liu Bo wrote: >>> On 08/01/2012 07:45 PM, Stefan Behrens wrote: >>>> With commit acce952b0, btrfs was changed to flag the filesystem with >>>> BTRFS_SUPER_FLAG_ERROR and switch to read-only mode after a fatal >>>> error happened like a write I/O errors of all mirrors. >>>> In such situations, on unmount, the superblock is written in >>>> btrfs_error_commit_super(). This is done with the intention to be able >>>> to evaluate the error flag on the next mount. A warning is printed >>>> in this case during the next mount and the log tree is ignored. >>>> >>>> The issue is that it is possible that the superblock points to a root >>>> that was not written (due to write I/O errors). >>>> The result is that the filesystem cannot be mounted. btrfsck also does >>>> not start and all the other btrfs-progs tools fail to start as well. >>>> However, mount -o recovery is working well and does the right things >>>> to recover the filesystem (i.e., don't use the log root, clear the >>>> free space cache and use the next mountable root that is stored in the >>>> root backup array). >>>> >>>> This patch removes the writing of the superblock when >>>> BTRFS_SUPER_FLAG_ERROR is set, and removes the handling of the error >>>> flag in the mount function. >>>> >>> >>> Yes, I have to admit that this can be a serious problem. >>> >>> But we'll need to send the error flag stored in the super block into >>> disk in the future so that the next mount can find it unstable and do >>> fsck by itself maybe. >> >> Hum, that's possible. However, I neither see >> >> a) a safe way to get that flag to disk >> >> nor >> >> b) a situation where this flag would help. When we abort a transaction, we just >> roll everything back to the last commit, i.e. a consistent state. So if we stop >> writing a potentially corrupt super block, we should be fine anyway. Or am I >> missing something? >> > > I'm just wondering if we can roll everything back well, why do we need fsck? If the disks support barriers, we roll everything back very well. The most recent superblock on the disks always defines a consistent filesystem state. There are only two remaining filesystem consistency issues left that can cause inconsistent states, one is the one that the patch in this email addresses, and the second one is that the error result from barrier_all_devices() is ignored (which I want to change next). -- 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 Wed, 01 Aug 2012 16:31:54 +0200, Stefan Behrens wrote: > On Wed, 01 Aug 2012 21:31:58 +0800, Liu Bo wrote: >> On 08/01/2012 09:07 PM, Jan Schmidt wrote: >>> On Wed, August 01, 2012 at 14:02 (+0200), Liu Bo wrote: >>>> On 08/01/2012 07:45 PM, Stefan Behrens wrote: >>>>> With commit acce952b0, btrfs was changed to flag the filesystem with >>>>> BTRFS_SUPER_FLAG_ERROR and switch to read-only mode after a fatal >>>>> error happened like a write I/O errors of all mirrors. >>>>> In such situations, on unmount, the superblock is written in >>>>> btrfs_error_commit_super(). This is done with the intention to be able >>>>> to evaluate the error flag on the next mount. A warning is printed >>>>> in this case during the next mount and the log tree is ignored. >>>>> >>>>> The issue is that it is possible that the superblock points to a root >>>>> that was not written (due to write I/O errors). >>>>> The result is that the filesystem cannot be mounted. btrfsck also does >>>>> not start and all the other btrfs-progs tools fail to start as well. >>>>> However, mount -o recovery is working well and does the right things >>>>> to recover the filesystem (i.e., don't use the log root, clear the >>>>> free space cache and use the next mountable root that is stored in the >>>>> root backup array). >>>>> >>>>> This patch removes the writing of the superblock when >>>>> BTRFS_SUPER_FLAG_ERROR is set, and removes the handling of the error >>>>> flag in the mount function. >>>>> >>>> >>>> Yes, I have to admit that this can be a serious problem. >>>> >>>> But we'll need to send the error flag stored in the super block into >>>> disk in the future so that the next mount can find it unstable and do >>>> fsck by itself maybe. >>> >>> Hum, that's possible. However, I neither see >>> >>> a) a safe way to get that flag to disk >>> >>> nor >>> >>> b) a situation where this flag would help. When we abort a transaction, we just >>> roll everything back to the last commit, i.e. a consistent state. So if we stop >>> writing a potentially corrupt super block, we should be fine anyway. Or am I >>> missing something? >>> >> >> I'm just wondering if we can roll everything back well, why do we need fsck? > > If the disks support barriers, we roll everything back very well. The > most recent superblock on the disks always defines a consistent > filesystem state. There are only two remaining filesystem consistency > issues left that can cause inconsistent states, one is the one that the > patch in this email addresses, and the second one is that the error > result from barrier_all_devices() is ignored (which I want to change next). Hi Liu Bo, Do you have any remaining objections to that 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 08/02/2012 06:30 PM, Stefan Behrens wrote: > On Wed, 01 Aug 2012 16:31:54 +0200, Stefan Behrens wrote: >> On Wed, 01 Aug 2012 21:31:58 +0800, Liu Bo wrote: >>> On 08/01/2012 09:07 PM, Jan Schmidt wrote: >>>> On Wed, August 01, 2012 at 14:02 (+0200), Liu Bo wrote: >>>>> On 08/01/2012 07:45 PM, Stefan Behrens wrote: >>>>>> With commit acce952b0, btrfs was changed to flag the filesystem with >>>>>> BTRFS_SUPER_FLAG_ERROR and switch to read-only mode after a fatal >>>>>> error happened like a write I/O errors of all mirrors. >>>>>> In such situations, on unmount, the superblock is written in >>>>>> btrfs_error_commit_super(). This is done with the intention to be able >>>>>> to evaluate the error flag on the next mount. A warning is printed >>>>>> in this case during the next mount and the log tree is ignored. >>>>>> >>>>>> The issue is that it is possible that the superblock points to a root >>>>>> that was not written (due to write I/O errors). >>>>>> The result is that the filesystem cannot be mounted. btrfsck also does >>>>>> not start and all the other btrfs-progs tools fail to start as well. >>>>>> However, mount -o recovery is working well and does the right things >>>>>> to recover the filesystem (i.e., don't use the log root, clear the >>>>>> free space cache and use the next mountable root that is stored in the >>>>>> root backup array). >>>>>> >>>>>> This patch removes the writing of the superblock when >>>>>> BTRFS_SUPER_FLAG_ERROR is set, and removes the handling of the error >>>>>> flag in the mount function. >>>>>> >>>>> >>>>> Yes, I have to admit that this can be a serious problem. >>>>> >>>>> But we'll need to send the error flag stored in the super block into >>>>> disk in the future so that the next mount can find it unstable and do >>>>> fsck by itself maybe. >>>> >>>> Hum, that's possible. However, I neither see >>>> >>>> a) a safe way to get that flag to disk >>>> >>>> nor >>>> >>>> b) a situation where this flag would help. When we abort a transaction, we just >>>> roll everything back to the last commit, i.e. a consistent state. So if we stop >>>> writing a potentially corrupt super block, we should be fine anyway. Or am I >>>> missing something? >>>> >>> >>> I'm just wondering if we can roll everything back well, why do we need fsck? >> >> If the disks support barriers, we roll everything back very well. The >> most recent superblock on the disks always defines a consistent >> filesystem state. There are only two remaining filesystem consistency >> issues left that can cause inconsistent states, one is the one that the >> patch in this email addresses, and the second one is that the error >> result from barrier_all_devices() is ignored (which I want to change next). > > Hi Liu Bo, > > Do you have any remaining objections to that patch? > Hi Stefan, Still I have another question: Our metadata can be flushed into disk if we reach the limit, 32k, so we can end up with updated metadata and the latest superblock if we do not write the current super block. Any ideas? thanks, liubo -- 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 02.08.2012 12:36, Liu Bo wrote: > On 08/02/2012 06:30 PM, Stefan Behrens wrote: >> On Wed, 01 Aug 2012 16:31:54 +0200, Stefan Behrens wrote: >>> On Wed, 01 Aug 2012 21:31:58 +0800, Liu Bo wrote: >>>> On 08/01/2012 09:07 PM, Jan Schmidt wrote: >>>>> On Wed, August 01, 2012 at 14:02 (+0200), Liu Bo wrote: >>>>>> On 08/01/2012 07:45 PM, Stefan Behrens wrote: >>>>>>> With commit acce952b0, btrfs was changed to flag the filesystem with >>>>>>> BTRFS_SUPER_FLAG_ERROR and switch to read-only mode after a fatal >>>>>>> error happened like a write I/O errors of all mirrors. >>>>>>> In such situations, on unmount, the superblock is written in >>>>>>> btrfs_error_commit_super(). This is done with the intention to be able >>>>>>> to evaluate the error flag on the next mount. A warning is printed >>>>>>> in this case during the next mount and the log tree is ignored. >>>>>>> >>>>>>> The issue is that it is possible that the superblock points to a root >>>>>>> that was not written (due to write I/O errors). >>>>>>> The result is that the filesystem cannot be mounted. btrfsck also does >>>>>>> not start and all the other btrfs-progs tools fail to start as well. >>>>>>> However, mount -o recovery is working well and does the right things >>>>>>> to recover the filesystem (i.e., don't use the log root, clear the >>>>>>> free space cache and use the next mountable root that is stored in the >>>>>>> root backup array). >>>>>>> >>>>>>> This patch removes the writing of the superblock when >>>>>>> BTRFS_SUPER_FLAG_ERROR is set, and removes the handling of the error >>>>>>> flag in the mount function. >>>>>>> >>>>>> >>>>>> Yes, I have to admit that this can be a serious problem. >>>>>> >>>>>> But we'll need to send the error flag stored in the super block into >>>>>> disk in the future so that the next mount can find it unstable and do >>>>>> fsck by itself maybe. >>>>> >>>>> Hum, that's possible. However, I neither see >>>>> >>>>> a) a safe way to get that flag to disk >>>>> >>>>> nor >>>>> >>>>> b) a situation where this flag would help. When we abort a transaction, we just >>>>> roll everything back to the last commit, i.e. a consistent state. So if we stop >>>>> writing a potentially corrupt super block, we should be fine anyway. Or am I >>>>> missing something? >>>>> >>>> >>>> I'm just wondering if we can roll everything back well, why do we need fsck? >>> >>> If the disks support barriers, we roll everything back very well. The >>> most recent superblock on the disks always defines a consistent >>> filesystem state. There are only two remaining filesystem consistency >>> issues left that can cause inconsistent states, one is the one that the >>> patch in this email addresses, and the second one is that the error >>> result from barrier_all_devices() is ignored (which I want to change next). >> >> Hi Liu Bo, >> >> Do you have any remaining objections to that patch? >> > > Hi Stefan, > > Still I have another question: > > Our metadata can be flushed into disk if we reach the limit, 32k, so we > can end up with updated metadata and the latest superblock if we do not > write the current super block. The old metadata stays valid until the new superblock is written, so no problem here, or maybe I don't understand your question :) > > Any ideas? > > thanks, > liubo > -- > 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 08/02/2012 07:18 PM, Arne Jansen wrote: > On 02.08.2012 12:36, Liu Bo wrote: >> On 08/02/2012 06:30 PM, Stefan Behrens wrote: >>> On Wed, 01 Aug 2012 16:31:54 +0200, Stefan Behrens wrote: >>>> On Wed, 01 Aug 2012 21:31:58 +0800, Liu Bo wrote: >>>>> On 08/01/2012 09:07 PM, Jan Schmidt wrote: >>>>>> On Wed, August 01, 2012 at 14:02 (+0200), Liu Bo wrote: >>>>>>> On 08/01/2012 07:45 PM, Stefan Behrens wrote: >>>>>>>> With commit acce952b0, btrfs was changed to flag the filesystem with >>>>>>>> BTRFS_SUPER_FLAG_ERROR and switch to read-only mode after a fatal >>>>>>>> error happened like a write I/O errors of all mirrors. >>>>>>>> In such situations, on unmount, the superblock is written in >>>>>>>> btrfs_error_commit_super(). This is done with the intention to be able >>>>>>>> to evaluate the error flag on the next mount. A warning is printed >>>>>>>> in this case during the next mount and the log tree is ignored. >>>>>>>> >>>>>>>> The issue is that it is possible that the superblock points to a root >>>>>>>> that was not written (due to write I/O errors). >>>>>>>> The result is that the filesystem cannot be mounted. btrfsck also does >>>>>>>> not start and all the other btrfs-progs tools fail to start as well. >>>>>>>> However, mount -o recovery is working well and does the right things >>>>>>>> to recover the filesystem (i.e., don't use the log root, clear the >>>>>>>> free space cache and use the next mountable root that is stored in the >>>>>>>> root backup array). >>>>>>>> >>>>>>>> This patch removes the writing of the superblock when >>>>>>>> BTRFS_SUPER_FLAG_ERROR is set, and removes the handling of the error >>>>>>>> flag in the mount function. >>>>>>>> >>>>>>> >>>>>>> Yes, I have to admit that this can be a serious problem. >>>>>>> >>>>>>> But we'll need to send the error flag stored in the super block into >>>>>>> disk in the future so that the next mount can find it unstable and do >>>>>>> fsck by itself maybe. >>>>>> >>>>>> Hum, that's possible. However, I neither see >>>>>> >>>>>> a) a safe way to get that flag to disk >>>>>> >>>>>> nor >>>>>> >>>>>> b) a situation where this flag would help. When we abort a transaction, we just >>>>>> roll everything back to the last commit, i.e. a consistent state. So if we stop >>>>>> writing a potentially corrupt super block, we should be fine anyway. Or am I >>>>>> missing something? >>>>>> >>>>> >>>>> I'm just wondering if we can roll everything back well, why do we need fsck? >>>> >>>> If the disks support barriers, we roll everything back very well. The >>>> most recent superblock on the disks always defines a consistent >>>> filesystem state. There are only two remaining filesystem consistency >>>> issues left that can cause inconsistent states, one is the one that the >>>> patch in this email addresses, and the second one is that the error >>>> result from barrier_all_devices() is ignored (which I want to change next). >>> >>> Hi Liu Bo, >>> >>> Do you have any remaining objections to that patch? >>> >> >> Hi Stefan, >> >> Still I have another question: >> >> Our metadata can be flushed into disk if we reach the limit, 32k, so we >> can end up with updated metadata and the latest superblock if we do not >> write the current super block. > > The old metadata stays valid until the new superblock is written, > so no problem here, or maybe I don't understand your question :) > Yeah, Arne, you're right :) But for undetected and unexpected errors as Arne had mentioned, I want to keep the error flag which is able to inform users that this FS is recommended (but not must) to do fsck at least. thanks, liubo >> >> Any ideas? >> >> thanks, >> liubo >> -- >> 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 02.08.2012 13:34, Liu Bo wrote: > On 08/02/2012 07:18 PM, Arne Jansen wrote: >> On 02.08.2012 12:36, Liu Bo wrote: >>> On 08/02/2012 06:30 PM, Stefan Behrens wrote: >>>> On Wed, 01 Aug 2012 16:31:54 +0200, Stefan Behrens wrote: >>>>> On Wed, 01 Aug 2012 21:31:58 +0800, Liu Bo wrote: >>>>>> On 08/01/2012 09:07 PM, Jan Schmidt wrote: >>>>>>> On Wed, August 01, 2012 at 14:02 (+0200), Liu Bo wrote: >>>>>>>> On 08/01/2012 07:45 PM, Stefan Behrens wrote: >>>>>>>>> With commit acce952b0, btrfs was changed to flag the filesystem with >>>>>>>>> BTRFS_SUPER_FLAG_ERROR and switch to read-only mode after a fatal >>>>>>>>> error happened like a write I/O errors of all mirrors. >>>>>>>>> In such situations, on unmount, the superblock is written in >>>>>>>>> btrfs_error_commit_super(). This is done with the intention to be able >>>>>>>>> to evaluate the error flag on the next mount. A warning is printed >>>>>>>>> in this case during the next mount and the log tree is ignored. >>>>>>>>> >>>>>>>>> The issue is that it is possible that the superblock points to a root >>>>>>>>> that was not written (due to write I/O errors). >>>>>>>>> The result is that the filesystem cannot be mounted. btrfsck also does >>>>>>>>> not start and all the other btrfs-progs tools fail to start as well. >>>>>>>>> However, mount -o recovery is working well and does the right things >>>>>>>>> to recover the filesystem (i.e., don't use the log root, clear the >>>>>>>>> free space cache and use the next mountable root that is stored in the >>>>>>>>> root backup array). >>>>>>>>> >>>>>>>>> This patch removes the writing of the superblock when >>>>>>>>> BTRFS_SUPER_FLAG_ERROR is set, and removes the handling of the error >>>>>>>>> flag in the mount function. >>>>>>>>> >>>>>>>> >>>>>>>> Yes, I have to admit that this can be a serious problem. >>>>>>>> >>>>>>>> But we'll need to send the error flag stored in the super block into >>>>>>>> disk in the future so that the next mount can find it unstable and do >>>>>>>> fsck by itself maybe. >>>>>>> >>>>>>> Hum, that's possible. However, I neither see >>>>>>> >>>>>>> a) a safe way to get that flag to disk >>>>>>> >>>>>>> nor >>>>>>> >>>>>>> b) a situation where this flag would help. When we abort a transaction, we just >>>>>>> roll everything back to the last commit, i.e. a consistent state. So if we stop >>>>>>> writing a potentially corrupt super block, we should be fine anyway. Or am I >>>>>>> missing something? >>>>>>> >>>>>> >>>>>> I'm just wondering if we can roll everything back well, why do we need fsck? >>>>> >>>>> If the disks support barriers, we roll everything back very well. The >>>>> most recent superblock on the disks always defines a consistent >>>>> filesystem state. There are only two remaining filesystem consistency >>>>> issues left that can cause inconsistent states, one is the one that the >>>>> patch in this email addresses, and the second one is that the error >>>>> result from barrier_all_devices() is ignored (which I want to change next). >>>> >>>> Hi Liu Bo, >>>> >>>> Do you have any remaining objections to that patch? >>>> >>> >>> Hi Stefan, >>> >>> Still I have another question: >>> >>> Our metadata can be flushed into disk if we reach the limit, 32k, so we >>> can end up with updated metadata and the latest superblock if we do not >>> write the current super block. >> >> The old metadata stays valid until the new superblock is written, >> so no problem here, or maybe I don't understand your question :) >> > > Yeah, Arne, you're right :) > > But for undetected and unexpected errors as Arne had mentioned, I want > to keep the error flag which is able to inform users that this FS is > recommended (but not must) to do fsck at least. How about storing the flag in a different location than the superblock? If the fs is in an unknown state, every write potentially makes it only worse. > > thanks, > liubo > >>> >>> Any ideas? >>> >>> thanks, >>> liubo >>> -- >>> 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 -- 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 08/02/2012 07:40 PM, Arne Jansen wrote: > On 02.08.2012 13:34, Liu Bo wrote: >> On 08/02/2012 07:18 PM, Arne Jansen wrote: >>> On 02.08.2012 12:36, Liu Bo wrote: >>>> On 08/02/2012 06:30 PM, Stefan Behrens wrote: >>>>> On Wed, 01 Aug 2012 16:31:54 +0200, Stefan Behrens wrote: >>>>>> On Wed, 01 Aug 2012 21:31:58 +0800, Liu Bo wrote: >>>>>>> On 08/01/2012 09:07 PM, Jan Schmidt wrote: >>>>>>>> On Wed, August 01, 2012 at 14:02 (+0200), Liu Bo wrote: >>>>>>>>> On 08/01/2012 07:45 PM, Stefan Behrens wrote: >>>>>>>>>> With commit acce952b0, btrfs was changed to flag the filesystem with >>>>>>>>>> BTRFS_SUPER_FLAG_ERROR and switch to read-only mode after a fatal >>>>>>>>>> error happened like a write I/O errors of all mirrors. >>>>>>>>>> In such situations, on unmount, the superblock is written in >>>>>>>>>> btrfs_error_commit_super(). This is done with the intention to be able >>>>>>>>>> to evaluate the error flag on the next mount. A warning is printed >>>>>>>>>> in this case during the next mount and the log tree is ignored. >>>>>>>>>> >>>>>>>>>> The issue is that it is possible that the superblock points to a root >>>>>>>>>> that was not written (due to write I/O errors). >>>>>>>>>> The result is that the filesystem cannot be mounted. btrfsck also does >>>>>>>>>> not start and all the other btrfs-progs tools fail to start as well. >>>>>>>>>> However, mount -o recovery is working well and does the right things >>>>>>>>>> to recover the filesystem (i.e., don't use the log root, clear the >>>>>>>>>> free space cache and use the next mountable root that is stored in the >>>>>>>>>> root backup array). >>>>>>>>>> >>>>>>>>>> This patch removes the writing of the superblock when >>>>>>>>>> BTRFS_SUPER_FLAG_ERROR is set, and removes the handling of the error >>>>>>>>>> flag in the mount function. >>>>>>>>>> >>>>>>>>> >>>>>>>>> Yes, I have to admit that this can be a serious problem. >>>>>>>>> >>>>>>>>> But we'll need to send the error flag stored in the super block into >>>>>>>>> disk in the future so that the next mount can find it unstable and do >>>>>>>>> fsck by itself maybe. >>>>>>>> >>>>>>>> Hum, that's possible. However, I neither see >>>>>>>> >>>>>>>> a) a safe way to get that flag to disk >>>>>>>> >>>>>>>> nor >>>>>>>> >>>>>>>> b) a situation where this flag would help. When we abort a transaction, we just >>>>>>>> roll everything back to the last commit, i.e. a consistent state. So if we stop >>>>>>>> writing a potentially corrupt super block, we should be fine anyway. Or am I >>>>>>>> missing something? >>>>>>>> >>>>>>> >>>>>>> I'm just wondering if we can roll everything back well, why do we need fsck? >>>>>> >>>>>> If the disks support barriers, we roll everything back very well. The >>>>>> most recent superblock on the disks always defines a consistent >>>>>> filesystem state. There are only two remaining filesystem consistency >>>>>> issues left that can cause inconsistent states, one is the one that the >>>>>> patch in this email addresses, and the second one is that the error >>>>>> result from barrier_all_devices() is ignored (which I want to change next). >>>>> >>>>> Hi Liu Bo, >>>>> >>>>> Do you have any remaining objections to that patch? >>>>> >>>> >>>> Hi Stefan, >>>> >>>> Still I have another question: >>>> >>>> Our metadata can be flushed into disk if we reach the limit, 32k, so we >>>> can end up with updated metadata and the latest superblock if we do not >>>> write the current super block. >>> >>> The old metadata stays valid until the new superblock is written, >>> so no problem here, or maybe I don't understand your question :) >>> >> >> Yeah, Arne, you're right :) >> >> But for undetected and unexpected errors as Arne had mentioned, I want >> to keep the error flag which is able to inform users that this FS is >> recommended (but not must) to do fsck at least. > > How about storing the flag in a different location than the superblock? > If the fs is in an unknown state, every write potentially makes it only > worse. > IMO it does not make sense if we don't write the flag into disk, and on ext4's side, it just tries to write the super block. Anyway, for now, our error flag has only been stored in memory, so what about just keep it until we find a graceful way? thanks, liubo >> >> thanks, >> liubo >> >>>> >>>> Any ideas? >>>> >>>> thanks, >>>> liubo >>>> -- >>>> 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 > -- 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 02.08.2012 13:57, Liu Bo wrote: > On 08/02/2012 07:40 PM, Arne Jansen wrote: >> On 02.08.2012 13:34, Liu Bo wrote: >>> On 08/02/2012 07:18 PM, Arne Jansen wrote: >>>> On 02.08.2012 12:36, Liu Bo wrote: >>>>> On 08/02/2012 06:30 PM, Stefan Behrens wrote: >>>>>> On Wed, 01 Aug 2012 16:31:54 +0200, Stefan Behrens wrote: >>>>>>> On Wed, 01 Aug 2012 21:31:58 +0800, Liu Bo wrote: >>>>>>>> On 08/01/2012 09:07 PM, Jan Schmidt wrote: >>>>>>>>> On Wed, August 01, 2012 at 14:02 (+0200), Liu Bo wrote: >>>>>>>>>> On 08/01/2012 07:45 PM, Stefan Behrens wrote: >>>>>>>>>>> With commit acce952b0, btrfs was changed to flag the filesystem with >>>>>>>>>>> BTRFS_SUPER_FLAG_ERROR and switch to read-only mode after a fatal >>>>>>>>>>> error happened like a write I/O errors of all mirrors. >>>>>>>>>>> In such situations, on unmount, the superblock is written in >>>>>>>>>>> btrfs_error_commit_super(). This is done with the intention to be able >>>>>>>>>>> to evaluate the error flag on the next mount. A warning is printed >>>>>>>>>>> in this case during the next mount and the log tree is ignored. >>>>>>>>>>> >>>>>>>>>>> The issue is that it is possible that the superblock points to a root >>>>>>>>>>> that was not written (due to write I/O errors). >>>>>>>>>>> The result is that the filesystem cannot be mounted. btrfsck also does >>>>>>>>>>> not start and all the other btrfs-progs tools fail to start as well. >>>>>>>>>>> However, mount -o recovery is working well and does the right things >>>>>>>>>>> to recover the filesystem (i.e., don't use the log root, clear the >>>>>>>>>>> free space cache and use the next mountable root that is stored in the >>>>>>>>>>> root backup array). >>>>>>>>>>> >>>>>>>>>>> This patch removes the writing of the superblock when >>>>>>>>>>> BTRFS_SUPER_FLAG_ERROR is set, and removes the handling of the error >>>>>>>>>>> flag in the mount function. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Yes, I have to admit that this can be a serious problem. >>>>>>>>>> >>>>>>>>>> But we'll need to send the error flag stored in the super block into >>>>>>>>>> disk in the future so that the next mount can find it unstable and do >>>>>>>>>> fsck by itself maybe. >>>>>>>>> >>>>>>>>> Hum, that's possible. However, I neither see >>>>>>>>> >>>>>>>>> a) a safe way to get that flag to disk >>>>>>>>> >>>>>>>>> nor >>>>>>>>> >>>>>>>>> b) a situation where this flag would help. When we abort a transaction, we just >>>>>>>>> roll everything back to the last commit, i.e. a consistent state. So if we stop >>>>>>>>> writing a potentially corrupt super block, we should be fine anyway. Or am I >>>>>>>>> missing something? >>>>>>>>> >>>>>>>> >>>>>>>> I'm just wondering if we can roll everything back well, why do we need fsck? >>>>>>> >>>>>>> If the disks support barriers, we roll everything back very well. The >>>>>>> most recent superblock on the disks always defines a consistent >>>>>>> filesystem state. There are only two remaining filesystem consistency >>>>>>> issues left that can cause inconsistent states, one is the one that the >>>>>>> patch in this email addresses, and the second one is that the error >>>>>>> result from barrier_all_devices() is ignored (which I want to change next). >>>>>> >>>>>> Hi Liu Bo, >>>>>> >>>>>> Do you have any remaining objections to that patch? >>>>>> >>>>> >>>>> Hi Stefan, >>>>> >>>>> Still I have another question: >>>>> >>>>> Our metadata can be flushed into disk if we reach the limit, 32k, so we >>>>> can end up with updated metadata and the latest superblock if we do not >>>>> write the current super block. >>>> >>>> The old metadata stays valid until the new superblock is written, >>>> so no problem here, or maybe I don't understand your question :) >>>> >>> >>> Yeah, Arne, you're right :) >>> >>> But for undetected and unexpected errors as Arne had mentioned, I want >>> to keep the error flag which is able to inform users that this FS is >>> recommended (but not must) to do fsck at least. >> >> How about storing the flag in a different location than the superblock? >> If the fs is in an unknown state, every write potentially makes it only >> worse. >> > > IMO it does not make sense if we don't write the flag into disk, and on > ext4's side, it just tries to write the super block. > > Anyway, for now, our error flag has only been stored in memory, so what > about just keep it until we find a graceful way? Yeah, we need this patch to restore consistency. We can define a fixed area on disk (e.g. behind the superblock) where we can write the flag to without risking the superblock. > > > thanks, > liubo > > >>> >>> thanks, >>> liubo >>> >>>>> >>>>> Any ideas? >>>>> >>>>> thanks, >>>>> liubo >>>>> -- >>>>> 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 >> > > -- > 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 Thu, Aug 02, 2012 at 03:46:50PM +0200, Arne Jansen wrote: > > Anyway, for now, our error flag has only been stored in memory, so what > > about just keep it until we find a graceful way? > > Yeah, we need this patch to restore consistency. We can define a fixed > area on disk (e.g. behind the superblock) where we can write the flag > to without risking the superblock. Is it possible that sectors around the superblock are somehow damaged and unwritable that writing to them will fail as well? Ie. some safe distance from the superblock would be needed. david -- 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 02.08.2012 15:57, David Sterba wrote: > On Thu, Aug 02, 2012 at 03:46:50PM +0200, Arne Jansen wrote: >>> Anyway, for now, our error flag has only been stored in memory, so what >>> about just keep it until we find a graceful way? >> >> Yeah, we need this patch to restore consistency. We can define a fixed >> area on disk (e.g. behind the superblock) where we can write the flag >> to without risking the superblock. > > Is it possible that sectors around the superblock are somehow damaged > and unwritable that writing to them will fail as well? Ie. some safe > distance from the superblock would be needed. In about 1MB distance you're side-by-side with the superblock, so closer to it might even be safer ;) > > david -- 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 Thu, August 02, 2012 at 15:46 (+0200), Arne Jansen wrote: > On 02.08.2012 13:57, Liu Bo wrote: >> Anyway, for now, our error flag has only been stored in memory, so what >> about just keep it until we find a graceful way? > > Yeah, we need this patch to restore consistency. We can define a fixed > area on disk (e.g. behind the superblock) where we can write the flag > to without risking the superblock. At least we all agree that we need this patch, fine. We don't yet agree that we need a place to store a "please consider fsck" flag. Can I please get one concrete example in which situation we a) do detect the user should really do a file system check AND b) do not abort the transaction to clean the mess up? (An example on how we could fail transaction cleanup is also accepted). If such a situation doesn't exist, there's no need for this flag. The fact that ext has such a flag doesn't convince me, probably because I know nothing about ext. I can imagine that they can detect file system errors without the ability to return to a potentially older consistent state. Thanks, -Jan -- 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 Thu, Aug 2, 2012 at 7:46 AM, Arne Jansen <sensille@gmx.net> wrote: > On 02.08.2012 13:57, Liu Bo wrote: >> On 08/02/2012 07:40 PM, Arne Jansen wrote: >>> On 02.08.2012 13:34, Liu Bo wrote: >>>> On 08/02/2012 07:18 PM, Arne Jansen wrote: >>>>> On 02.08.2012 12:36, Liu Bo wrote: >>>>>> On 08/02/2012 06:30 PM, Stefan Behrens wrote: >>>>>>> On Wed, 01 Aug 2012 16:31:54 +0200, Stefan Behrens wrote: >>>>>>>> On Wed, 01 Aug 2012 21:31:58 +0800, Liu Bo wrote: >>>>>>>>> On 08/01/2012 09:07 PM, Jan Schmidt wrote: >>>>>>>>>> On Wed, August 01, 2012 at 14:02 (+0200), Liu Bo wrote: >>>>>>>>>>> On 08/01/2012 07:45 PM, Stefan Behrens wrote: >>>>>>>>>>>> With commit acce952b0, btrfs was changed to flag the filesystem with >>>>>>>>>>>> BTRFS_SUPER_FLAG_ERROR and switch to read-only mode after a fatal >>>>>>>>>>>> error happened like a write I/O errors of all mirrors. >>>>>>>>>>>> In such situations, on unmount, the superblock is written in >>>>>>>>>>>> btrfs_error_commit_super(). This is done with the intention to be able >>>>>>>>>>>> to evaluate the error flag on the next mount. A warning is printed >>>>>>>>>>>> in this case during the next mount and the log tree is ignored. >>>>>>>>>>>> >>>>>>>>>>>> The issue is that it is possible that the superblock points to a root >>>>>>>>>>>> that was not written (due to write I/O errors). >>>>>>>>>>>> The result is that the filesystem cannot be mounted. btrfsck also does >>>>>>>>>>>> not start and all the other btrfs-progs tools fail to start as well. >>>>>>>>>>>> However, mount -o recovery is working well and does the right things >>>>>>>>>>>> to recover the filesystem (i.e., don't use the log root, clear the >>>>>>>>>>>> free space cache and use the next mountable root that is stored in the >>>>>>>>>>>> root backup array). >>>>>>>>>>>> >>>>>>>>>>>> This patch removes the writing of the superblock when >>>>>>>>>>>> BTRFS_SUPER_FLAG_ERROR is set, and removes the handling of the error >>>>>>>>>>>> flag in the mount function. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Yes, I have to admit that this can be a serious problem. >>>>>>>>>>> >>>>>>>>>>> But we'll need to send the error flag stored in the super block into >>>>>>>>>>> disk in the future so that the next mount can find it unstable and do >>>>>>>>>>> fsck by itself maybe. >>>>>>>>>> >>>>>>>>>> Hum, that's possible. However, I neither see >>>>>>>>>> >>>>>>>>>> a) a safe way to get that flag to disk >>>>>>>>>> >>>>>>>>>> nor >>>>>>>>>> >>>>>>>>>> b) a situation where this flag would help. When we abort a transaction, we just >>>>>>>>>> roll everything back to the last commit, i.e. a consistent state. So if we stop >>>>>>>>>> writing a potentially corrupt super block, we should be fine anyway. Or am I >>>>>>>>>> missing something? >>>>>>>>>> >>>>>>>>> >>>>>>>>> I'm just wondering if we can roll everything back well, why do we need fsck? >>>>>>>> >>>>>>>> If the disks support barriers, we roll everything back very well. The >>>>>>>> most recent superblock on the disks always defines a consistent >>>>>>>> filesystem state. There are only two remaining filesystem consistency >>>>>>>> issues left that can cause inconsistent states, one is the one that the >>>>>>>> patch in this email addresses, and the second one is that the error >>>>>>>> result from barrier_all_devices() is ignored (which I want to change next). >>>>>>> >>>>>>> Hi Liu Bo, >>>>>>> >>>>>>> Do you have any remaining objections to that patch? >>>>>>> >>>>>> >>>>>> Hi Stefan, >>>>>> >>>>>> Still I have another question: >>>>>> >>>>>> Our metadata can be flushed into disk if we reach the limit, 32k, so we >>>>>> can end up with updated metadata and the latest superblock if we do not >>>>>> write the current super block. >>>>> >>>>> The old metadata stays valid until the new superblock is written, >>>>> so no problem here, or maybe I don't understand your question :) >>>>> >>>> >>>> Yeah, Arne, you're right :) >>>> >>>> But for undetected and unexpected errors as Arne had mentioned, I want >>>> to keep the error flag which is able to inform users that this FS is >>>> recommended (but not must) to do fsck at least. >>> >>> How about storing the flag in a different location than the superblock? >>> If the fs is in an unknown state, every write potentially makes it only >>> worse. >>> >> >> IMO it does not make sense if we don't write the flag into disk, and on >> ext4's side, it just tries to write the super block. >> >> Anyway, for now, our error flag has only been stored in memory, so what >> about just keep it until we find a graceful way? > > Yeah, we need this patch to restore consistency. We can define a fixed > area on disk (e.g. behind the superblock) where we can write the flag > to without risking the superblock. Is there a reason btrfs_error_commit_super couldn't do the as treelog: update only the first superblock via max_mirrors=1? I'd expect that fsck, -o recovery and so forth should all handle this correctly already, and we even have documentation that discusses it. -- 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 08/02/2012 10:28 PM, Jan Schmidt wrote: > On Thu, August 02, 2012 at 15:46 (+0200), Arne Jansen wrote: >> On 02.08.2012 13:57, Liu Bo wrote: >>> Anyway, for now, our error flag has only been stored in memory, so what >>> about just keep it until we find a graceful way? >> >> Yeah, we need this patch to restore consistency. We can define a fixed >> area on disk (e.g. behind the superblock) where we can write the flag >> to without risking the superblock. > > At least we all agree that we need this patch, fine. > > We don't yet agree that we need a place to store a "please consider fsck" flag. > Can I please get one concrete example in which situation we > > a) do detect the user should really do a file system check AND > b) do not abort the transaction to clean the mess up? > > (An example on how we could fail transaction cleanup is also accepted). > Unfortunately I don't have such an example either. Since we always get COW on metadata, I believe that it's ok to just roll back on failure. > If such a situation doesn't exist, there's no need for this flag. The fact that > ext has such a flag doesn't convince me, probably because I know nothing about > ext. I can imagine that they can detect file system errors without the ability > to return to a potentially older consistent state. > This error flag is also used to indicate filesystem's error state for transaction cleanup, so keeping it in memory is reasonable. thanks, liubo > Thanks, > -Jan > -- 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
[added Josef] Josef: Please queue this patch for rc2, we finally agreed. On Fri, August 03, 2012 at 05:20 (+0200), Liu Bo wrote: > On 08/02/2012 10:28 PM, Jan Schmidt wrote: >> If such a situation doesn't exist, there's no need for this flag. The fact that >> ext has such a flag doesn't convince me, probably because I know nothing about >> ext. I can imagine that they can detect file system errors without the ability >> to return to a potentially older consistent state. >> > > This error flag is also used to indicate filesystem's error state for > transaction cleanup, so keeping it in memory is reasonable. Granted :-) -Jan -- 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 502b20c..f6a1d0f 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2531,8 +2531,7 @@ retry_root_backup: goto fail_trans_kthread; /* do not make disk changes in broken FS */ - if (btrfs_super_log_root(disk_super) != 0 && - !(fs_info->fs_state & BTRFS_SUPER_FLAG_ERROR)) { + if (btrfs_super_log_root(disk_super) != 0) { u64 bytenr = btrfs_super_log_root(disk_super); if (fs_devices->rw_devices == 0) { @@ -3192,30 +3191,14 @@ int close_ctree(struct btrfs_root *root) /* clear out the rbtree of defraggable inodes */ btrfs_run_defrag_inodes(fs_info); - /* - * Here come 2 situations when btrfs is broken to flip readonly: - * - * 1. when btrfs flips readonly somewhere else before - * btrfs_commit_super, sb->s_flags has MS_RDONLY flag, - * and btrfs will skip to write sb directly to keep - * ERROR state on disk. - * - * 2. when btrfs flips readonly just in btrfs_commit_super, - * and in such case, btrfs cannot write sb via btrfs_commit_super, - * and since fs_state has been set BTRFS_SUPER_FLAG_ERROR flag, - * btrfs will cleanup all FS resources first and write sb then. - */ if (!(fs_info->sb->s_flags & MS_RDONLY)) { ret = btrfs_commit_super(root); if (ret) printk(KERN_ERR "btrfs: commit super ret %d\n", ret); } - if (fs_info->fs_state & BTRFS_SUPER_FLAG_ERROR) { - ret = btrfs_error_commit_super(root); - if (ret) - printk(KERN_ERR "btrfs: commit super ret %d\n", ret); - } + if (fs_info->fs_state & BTRFS_SUPER_FLAG_ERROR) + btrfs_error_commit_super(root); btrfs_put_block_group_cache(fs_info); @@ -3437,18 +3420,11 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, if (read_only) return 0; - if (fs_info->fs_state & BTRFS_SUPER_FLAG_ERROR) { - printk(KERN_WARNING "warning: mount fs with errors, " - "running btrfsck is recommended\n"); - } - return 0; } -int btrfs_error_commit_super(struct btrfs_root *root) +void btrfs_error_commit_super(struct btrfs_root *root) { - int ret; - mutex_lock(&root->fs_info->cleaner_mutex); btrfs_run_delayed_iputs(root); mutex_unlock(&root->fs_info->cleaner_mutex); @@ -3458,10 +3434,6 @@ int btrfs_error_commit_super(struct btrfs_root *root) /* cleanup FS via transaction */ btrfs_cleanup_transaction(root); - - ret = write_ctree_super(NULL, root, 0); - - return ret; } static void btrfs_destroy_ordered_operations(struct btrfs_root *root) diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h index 95e147e..c5b00a7 100644 --- a/fs/btrfs/disk-io.h +++ b/fs/btrfs/disk-io.h @@ -54,7 +54,7 @@ int write_ctree_super(struct btrfs_trans_handle *trans, struct btrfs_root *root, int max_mirrors); struct buffer_head *btrfs_read_dev_super(struct block_device *bdev); int btrfs_commit_super(struct btrfs_root *root); -int btrfs_error_commit_super(struct btrfs_root *root); +void btrfs_error_commit_super(struct btrfs_root *root); struct extent_buffer *btrfs_find_tree_block(struct btrfs_root *root, u64 bytenr, u32 blocksize); struct btrfs_root *btrfs_read_fs_root_no_radix(struct btrfs_root *tree_root,