| Message ID | 1446013561-22121-5-git-send-email-ghe@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Gang, This is not like a right patch. First, online file check only checks inode's block number, valid flag, fs generation value, and meta ecc. I never see a real corruption happened only on this field, if these fields are corrupted, that means something bad may happen on other place. So fix this field may not help and even cause corruption more hard. Second, the repair way is wrong. In ocfs2_filecheck_repair_inode_block(), if these fields in disk don't match the ones in memory, the ones in memory are used to update the disk fields. The question is how do you know these field in memory are right(they may be the real corrupted ones)? Thanks, Junxiao. On 10/28/2015 02:26 PM, Gang He wrote: > +static int ocfs2_filecheck_repair_inode_block(struct super_block *sb, > + struct buffer_head *bh) > +{ > + int rc; > + int changed = 0; > + struct ocfs2_dinode *di = (struct ocfs2_dinode *)bh->b_data; > + > + rc = ocfs2_filecheck_validate_inode_block(sb, bh); > + /* Can't fix invalid inode block */ > + if (!rc || rc == -OCFS2_FILECHECK_ERR_INVALIDINO) > + return rc; > + > + trace_ocfs2_filecheck_repair_inode_block( > + (unsigned long long)bh->b_blocknr); > + > + if (ocfs2_is_hard_readonly(OCFS2_SB(sb)) || > + ocfs2_is_soft_readonly(OCFS2_SB(sb))) { > + mlog(ML_ERROR, > + "Filecheck: try to repair dinode #%llu on readonly filesystem\n", > + (unsigned long long)bh->b_blocknr); > + return -OCFS2_FILECHECK_ERR_READONLY; > + } > + > + if (le64_to_cpu(di->i_blkno) != bh->b_blocknr) { > + di->i_blkno = cpu_to_le64(bh->b_blocknr); > + changed = 1; > + mlog(ML_ERROR, > + "Filecheck: reset dinode #%llu: i_blkno to %llu\n", > + (unsigned long long)bh->b_blocknr, > + (unsigned long long)le64_to_cpu(di->i_blkno)); > + } > + > + if (!(di->i_flags & cpu_to_le32(OCFS2_VALID_FL))) { > + di->i_flags |= cpu_to_le32(OCFS2_VALID_FL); > + changed = 1; > + mlog(ML_ERROR, > + "Filecheck: reset dinode #%llu: OCFS2_VALID_FL is set\n", > + (unsigned long long)bh->b_blocknr); > + } > + > + if (le32_to_cpu(di->i_fs_generation) != > + OCFS2_SB(sb)->fs_generation) { > + di->i_fs_generation = cpu_to_le32(OCFS2_SB(sb)->fs_generation); > + changed = 1; > + mlog(ML_ERROR, > + "Filecheck: reset dinode #%llu: fs_generation to %u\n", > + (unsigned long long)bh->b_blocknr, > + le32_to_cpu(di->i_fs_generation)); > + } > + > + if (changed || > + ocfs2_validate_meta_ecc(sb, bh->b_data, &di->i_check)) { > + ocfs2_compute_meta_ecc(sb, bh->b_data, &di->i_check); > + mark_buffer_dirty(bh); > + mlog(ML_ERROR, > + "Filecheck: reset dinode #%llu: compute meta ecc\n", > + (unsigned long long)bh->b_blocknr); > + } > + > + return 0; > +}
Hello Junxiao, See my comments inline. >>> > Hi Gang, > > This is not like a right patch. > First, online file check only checks inode's block number, valid flag, > fs generation value, and meta ecc. I never see a real corruption > happened only on this field, if these fields are corrupted, that means > something bad may happen on other place. So fix this field may not help > and even cause corruption more hard. This online file check/fix feature is used to check/fix some light file meta block corruption, instead of turning a file system off and using fsck.ocfs2. e.g. meta ecc error, we really need not to use fsck.ocfs2. of course, this feature does not replace fsck.ocfs2 and touch some complicated meta block problems, if there is some potential problem in some areas, we can discuss them one by one. > Second, the repair way is wrong. In > ocfs2_filecheck_repair_inode_block(), if these fields in disk don't > match the ones in memory, the ones in memory are used to update the disk > fields. The question is how do you know these field in memory are > right(they may be the real corrupted ones)? Here, if the inode block was corrupted, the file system is not able to load it into the memory. ocfs2_filecheck_repair_inode_block() will able to load it into the memory, since it try to fix these light-level problem before loading. if the fix is OK, the changed meta-block can pass the block-validate function and load into the memory as a inode object. Since the file system is under a cluster environment, we have to use some existing function and code path to keep these block operation under a cluster lock. Thanks Gang > > Thanks, > Junxiao. > On 10/28/2015 02:26 PM, Gang He wrote: >> +static int ocfs2_filecheck_repair_inode_block(struct super_block *sb, >> + struct buffer_head *bh) >> +{ >> + int rc; >> + int changed = 0; >> + struct ocfs2_dinode *di = (struct ocfs2_dinode *)bh->b_data; >> + >> + rc = ocfs2_filecheck_validate_inode_block(sb, bh); >> + /* Can't fix invalid inode block */ >> + if (!rc || rc == -OCFS2_FILECHECK_ERR_INVALIDINO) >> + return rc; >> + >> + trace_ocfs2_filecheck_repair_inode_block( >> + (unsigned long long)bh->b_blocknr); >> + >> + if (ocfs2_is_hard_readonly(OCFS2_SB(sb)) || >> + ocfs2_is_soft_readonly(OCFS2_SB(sb))) { >> + mlog(ML_ERROR, >> + "Filecheck: try to repair dinode #%llu on readonly filesystem\n", >> + (unsigned long long)bh->b_blocknr); >> + return -OCFS2_FILECHECK_ERR_READONLY; >> + } >> + >> + if (le64_to_cpu(di->i_blkno) != bh->b_blocknr) { >> + di->i_blkno = cpu_to_le64(bh->b_blocknr); >> + changed = 1; >> + mlog(ML_ERROR, >> + "Filecheck: reset dinode #%llu: i_blkno to %llu\n", >> + (unsigned long long)bh->b_blocknr, >> + (unsigned long long)le64_to_cpu(di->i_blkno)); >> + } >> + >> + if (!(di->i_flags & cpu_to_le32(OCFS2_VALID_FL))) { >> + di->i_flags |= cpu_to_le32(OCFS2_VALID_FL); >> + changed = 1; >> + mlog(ML_ERROR, >> + "Filecheck: reset dinode #%llu: OCFS2_VALID_FL is set\n", >> + (unsigned long long)bh->b_blocknr); >> + } >> + >> + if (le32_to_cpu(di->i_fs_generation) != >> + OCFS2_SB(sb)->fs_generation) { >> + di->i_fs_generation = cpu_to_le32(OCFS2_SB(sb)->fs_generation); >> + changed = 1; >> + mlog(ML_ERROR, >> + "Filecheck: reset dinode #%llu: fs_generation to %u\n", >> + (unsigned long long)bh->b_blocknr, >> + le32_to_cpu(di->i_fs_generation)); >> + } >> + >> + if (changed || >> + ocfs2_validate_meta_ecc(sb, bh->b_data, &di->i_check)) { >> + ocfs2_compute_meta_ecc(sb, bh->b_data, &di->i_check); >> + mark_buffer_dirty(bh); >> + mlog(ML_ERROR, >> + "Filecheck: reset dinode #%llu: compute meta ecc\n", >> + (unsigned long long)bh->b_blocknr); >> + } >> + >> + return 0; >> +}
On 11/03/2015 04:15 PM, Gang He wrote: > Hello Junxiao, > > See my comments inline. > > >>>> >> Hi Gang, >> >> This is not like a right patch. >> First, online file check only checks inode's block number, valid flag, >> fs generation value, and meta ecc. I never see a real corruption >> happened only on this field, if these fields are corrupted, that means >> something bad may happen on other place. So fix this field may not help >> and even cause corruption more hard. > This online file check/fix feature is used to check/fix some light file meta block corruption, instead of turning a file system off and using fsck.ocfs2. What's light meta block corruption? Do you have a case about it? > e.g. meta ecc error, we really need not to use fsck.ocfs2. > of course, this feature does not replace fsck.ocfs2 and touch some complicated meta block problems, if there is some potential problem in some areas, we can discuss them one by one. > > > >> Second, the repair way is wrong. In >> ocfs2_filecheck_repair_inode_block(), if these fields in disk don't >> match the ones in memory, the ones in memory are used to update the disk >> fields. The question is how do you know these field in memory are >> right(they may be the real corrupted ones)? > Here, if the inode block was corrupted, the file system is not able to load it into the memory. How do you know inode block corrupted? If bh for inode block is overwritten, i mean bh corrupted, the repair will corrupted a good inode block. Thanks, Junxiao. > ocfs2_filecheck_repair_inode_block() will able to load it into the memory, since it try to fix these light-level problem before loading. > if the fix is OK, the changed meta-block can pass the block-validate function and load into the memory as a inode object. > Since the file system is under a cluster environment, we have to use some existing function and code path to keep these block operation under a cluster lock. > > > Thanks > Gang > >> >> Thanks, >> Junxiao. >> On 10/28/2015 02:26 PM, Gang He wrote: >>> +static int ocfs2_filecheck_repair_inode_block(struct super_block *sb, >>> + struct buffer_head *bh) >>> +{ >>> + int rc; >>> + int changed = 0; >>> + struct ocfs2_dinode *di = (struct ocfs2_dinode *)bh->b_data; >>> + >>> + rc = ocfs2_filecheck_validate_inode_block(sb, bh); >>> + /* Can't fix invalid inode block */ >>> + if (!rc || rc == -OCFS2_FILECHECK_ERR_INVALIDINO) >>> + return rc; >>> + >>> + trace_ocfs2_filecheck_repair_inode_block( >>> + (unsigned long long)bh->b_blocknr); >>> + >>> + if (ocfs2_is_hard_readonly(OCFS2_SB(sb)) || >>> + ocfs2_is_soft_readonly(OCFS2_SB(sb))) { >>> + mlog(ML_ERROR, >>> + "Filecheck: try to repair dinode #%llu on readonly filesystem\n", >>> + (unsigned long long)bh->b_blocknr); >>> + return -OCFS2_FILECHECK_ERR_READONLY; >>> + } >>> + >>> + if (le64_to_cpu(di->i_blkno) != bh->b_blocknr) { >>> + di->i_blkno = cpu_to_le64(bh->b_blocknr); >>> + changed = 1; >>> + mlog(ML_ERROR, >>> + "Filecheck: reset dinode #%llu: i_blkno to %llu\n", >>> + (unsigned long long)bh->b_blocknr, >>> + (unsigned long long)le64_to_cpu(di->i_blkno)); >>> + } >>> + >>> + if (!(di->i_flags & cpu_to_le32(OCFS2_VALID_FL))) { >>> + di->i_flags |= cpu_to_le32(OCFS2_VALID_FL); >>> + changed = 1; >>> + mlog(ML_ERROR, >>> + "Filecheck: reset dinode #%llu: OCFS2_VALID_FL is set\n", >>> + (unsigned long long)bh->b_blocknr); >>> + } >>> + >>> + if (le32_to_cpu(di->i_fs_generation) != >>> + OCFS2_SB(sb)->fs_generation) { >>> + di->i_fs_generation = cpu_to_le32(OCFS2_SB(sb)->fs_generation); >>> + changed = 1; >>> + mlog(ML_ERROR, >>> + "Filecheck: reset dinode #%llu: fs_generation to %u\n", >>> + (unsigned long long)bh->b_blocknr, >>> + le32_to_cpu(di->i_fs_generation)); >>> + } >>> + >>> + if (changed || >>> + ocfs2_validate_meta_ecc(sb, bh->b_data, &di->i_check)) { >>> + ocfs2_compute_meta_ecc(sb, bh->b_data, &di->i_check); >>> + mark_buffer_dirty(bh); >>> + mlog(ML_ERROR, >>> + "Filecheck: reset dinode #%llu: compute meta ecc\n", >>> + (unsigned long long)bh->b_blocknr); >>> + } >>> + >>> + return 0; >>> +} >
>>> > On 11/03/2015 04:15 PM, Gang He wrote: >> Hello Junxiao, >> >> See my comments inline. >> >> >>>>> >>> Hi Gang, >>> >>> This is not like a right patch. >>> First, online file check only checks inode's block number, valid flag, >>> fs generation value, and meta ecc. I never see a real corruption >>> happened only on this field, if these fields are corrupted, that means >>> something bad may happen on other place. So fix this field may not help >>> and even cause corruption more hard. >> This online file check/fix feature is used to check/fix some light file meta > block corruption, instead of turning a file system off and using fsck.ocfs2. > What's light meta block corruption? Do you have a case about it? >> e.g. meta ecc error, we really need not to use fsck.ocfs2. >> of course, this feature does not replace fsck.ocfs2 and touch some > complicated meta block problems, if there is some potential problem in some > areas, we can discuss them one by one. >> >> >> >>> Second, the repair way is wrong. In >>> ocfs2_filecheck_repair_inode_block(), if these fields in disk don't >>> match the ones in memory, the ones in memory are used to update the disk >>> fields. The question is how do you know these field in memory are >>> right(they may be the real corrupted ones)? >> Here, if the inode block was corrupted, the file system is not able to load > it into the memory. > How do you know inode block corrupted? If bh for inode block is > overwritten, i mean bh corrupted, the repair will corrupted a good inode > block. You know, the meta block is only validated when the file system loads the block from disk to memory. If the inode object is in the memory, we consider this inode block is OK. If the inode is not loaded by the file system via the normal way, the file system will print a kernel error log to tell which ino is corrupted. we will use ocfs2_filecheck_repair_inode_block() function to fix the inode block before loading. Thanks Gang > > Thanks, > Junxiao. > >> ocfs2_filecheck_repair_inode_block() will able to load it into the memory, > since it try to fix these light-level problem before loading. >> if the fix is OK, the changed meta-block can pass the block-validate function > and load into the memory as a inode object. >> Since the file system is under a cluster environment, we have to use some > existing function and code path to keep these block operation under a cluster > lock. >> >> >> Thanks >> Gang >> >>> >>> Thanks, >>> Junxiao. >>> On 10/28/2015 02:26 PM, Gang He wrote: >>>> +static int ocfs2_filecheck_repair_inode_block(struct super_block *sb, >>>> + struct buffer_head *bh) >>>> +{ >>>> + int rc; >>>> + int changed = 0; >>>> + struct ocfs2_dinode *di = (struct ocfs2_dinode *)bh->b_data; >>>> + >>>> + rc = ocfs2_filecheck_validate_inode_block(sb, bh); >>>> + /* Can't fix invalid inode block */ >>>> + if (!rc || rc == -OCFS2_FILECHECK_ERR_INVALIDINO) >>>> + return rc; >>>> + >>>> + trace_ocfs2_filecheck_repair_inode_block( >>>> + (unsigned long long)bh->b_blocknr); >>>> + >>>> + if (ocfs2_is_hard_readonly(OCFS2_SB(sb)) || >>>> + ocfs2_is_soft_readonly(OCFS2_SB(sb))) { >>>> + mlog(ML_ERROR, >>>> + "Filecheck: try to repair dinode #%llu on readonly filesystem\n", >>>> + (unsigned long long)bh->b_blocknr); >>>> + return -OCFS2_FILECHECK_ERR_READONLY; >>>> + } >>>> + >>>> + if (le64_to_cpu(di->i_blkno) != bh->b_blocknr) { >>>> + di->i_blkno = cpu_to_le64(bh->b_blocknr); >>>> + changed = 1; >>>> + mlog(ML_ERROR, >>>> + "Filecheck: reset dinode #%llu: i_blkno to %llu\n", >>>> + (unsigned long long)bh->b_blocknr, >>>> + (unsigned long long)le64_to_cpu(di->i_blkno)); >>>> + } >>>> + >>>> + if (!(di->i_flags & cpu_to_le32(OCFS2_VALID_FL))) { >>>> + di->i_flags |= cpu_to_le32(OCFS2_VALID_FL); >>>> + changed = 1; >>>> + mlog(ML_ERROR, >>>> + "Filecheck: reset dinode #%llu: OCFS2_VALID_FL is set\n", >>>> + (unsigned long long)bh->b_blocknr); >>>> + } >>>> + >>>> + if (le32_to_cpu(di->i_fs_generation) != >>>> + OCFS2_SB(sb)->fs_generation) { >>>> + di->i_fs_generation = cpu_to_le32(OCFS2_SB(sb)->fs_generation); >>>> + changed = 1; >>>> + mlog(ML_ERROR, >>>> + "Filecheck: reset dinode #%llu: fs_generation to %u\n", >>>> + (unsigned long long)bh->b_blocknr, >>>> + le32_to_cpu(di->i_fs_generation)); >>>> + } >>>> + >>>> + if (changed || >>>> + ocfs2_validate_meta_ecc(sb, bh->b_data, &di->i_check)) { >>>> + ocfs2_compute_meta_ecc(sb, bh->b_data, &di->i_check); >>>> + mark_buffer_dirty(bh); >>>> + mlog(ML_ERROR, >>>> + "Filecheck: reset dinode #%llu: compute meta ecc\n", >>>> + (unsigned long long)bh->b_blocknr); >>>> + } >>>> + >>>> + return 0; >>>> +} >>
On 11/03/2015 04:47 PM, Gang He wrote: > > > >>>> >> On 11/03/2015 04:15 PM, Gang He wrote: >>> Hello Junxiao, >>> >>> See my comments inline. >>> >>> >>>>>> >>>> Hi Gang, >>>> >>>> This is not like a right patch. >>>> First, online file check only checks inode's block number, valid flag, >>>> fs generation value, and meta ecc. I never see a real corruption >>>> happened only on this field, if these fields are corrupted, that means >>>> something bad may happen on other place. So fix this field may not help >>>> and even cause corruption more hard. >>> This online file check/fix feature is used to check/fix some light file meta >> block corruption, instead of turning a file system off and using fsck.ocfs2. >> What's light meta block corruption? Do you have a case about it? >>> e.g. meta ecc error, we really need not to use fsck.ocfs2. >>> of course, this feature does not replace fsck.ocfs2 and touch some >> complicated meta block problems, if there is some potential problem in some >> areas, we can discuss them one by one. >>> >>> >>> >>>> Second, the repair way is wrong. In >>>> ocfs2_filecheck_repair_inode_block(), if these fields in disk don't >>>> match the ones in memory, the ones in memory are used to update the disk >>>> fields. The question is how do you know these field in memory are >>>> right(they may be the real corrupted ones)? >>> Here, if the inode block was corrupted, the file system is not able to load >> it into the memory. >> How do you know inode block corrupted? If bh for inode block is >> overwritten, i mean bh corrupted, the repair will corrupted a good inode >> block. > You know, the meta block is only validated when the file system loads the block from disk to memory. > If the inode object is in the memory, we consider this inode block is OK. This assuming is not true as there are always bugs. Bugs can make inode object in memory bad and corrupted the fs when repair the inode. Thanks, Junxiao. > If the inode is not loaded by the file system via the normal way, the file system will print a kernel error log to tell which ino is corrupted. > we will use ocfs2_filecheck_repair_inode_block() function to fix the inode block before loading. > > Thanks > Gang > >> >> Thanks, >> Junxiao. >> >>> ocfs2_filecheck_repair_inode_block() will able to load it into the memory, >> since it try to fix these light-level problem before loading. >>> if the fix is OK, the changed meta-block can pass the block-validate function >> and load into the memory as a inode object. >>> Since the file system is under a cluster environment, we have to use some >> existing function and code path to keep these block operation under a cluster >> lock. >>> >>> >>> Thanks >>> Gang >>> >>>> >>>> Thanks, >>>> Junxiao. >>>> On 10/28/2015 02:26 PM, Gang He wrote: >>>>> +static int ocfs2_filecheck_repair_inode_block(struct super_block *sb, >>>>> + struct buffer_head *bh) >>>>> +{ >>>>> + int rc; >>>>> + int changed = 0; >>>>> + struct ocfs2_dinode *di = (struct ocfs2_dinode *)bh->b_data; >>>>> + >>>>> + rc = ocfs2_filecheck_validate_inode_block(sb, bh); >>>>> + /* Can't fix invalid inode block */ >>>>> + if (!rc || rc == -OCFS2_FILECHECK_ERR_INVALIDINO) >>>>> + return rc; >>>>> + >>>>> + trace_ocfs2_filecheck_repair_inode_block( >>>>> + (unsigned long long)bh->b_blocknr); >>>>> + >>>>> + if (ocfs2_is_hard_readonly(OCFS2_SB(sb)) || >>>>> + ocfs2_is_soft_readonly(OCFS2_SB(sb))) { >>>>> + mlog(ML_ERROR, >>>>> + "Filecheck: try to repair dinode #%llu on readonly filesystem\n", >>>>> + (unsigned long long)bh->b_blocknr); >>>>> + return -OCFS2_FILECHECK_ERR_READONLY; >>>>> + } >>>>> + >>>>> + if (le64_to_cpu(di->i_blkno) != bh->b_blocknr) { >>>>> + di->i_blkno = cpu_to_le64(bh->b_blocknr); >>>>> + changed = 1; >>>>> + mlog(ML_ERROR, >>>>> + "Filecheck: reset dinode #%llu: i_blkno to %llu\n", >>>>> + (unsigned long long)bh->b_blocknr, >>>>> + (unsigned long long)le64_to_cpu(di->i_blkno)); >>>>> + } >>>>> + >>>>> + if (!(di->i_flags & cpu_to_le32(OCFS2_VALID_FL))) { >>>>> + di->i_flags |= cpu_to_le32(OCFS2_VALID_FL); >>>>> + changed = 1; >>>>> + mlog(ML_ERROR, >>>>> + "Filecheck: reset dinode #%llu: OCFS2_VALID_FL is set\n", >>>>> + (unsigned long long)bh->b_blocknr); >>>>> + } >>>>> + >>>>> + if (le32_to_cpu(di->i_fs_generation) != >>>>> + OCFS2_SB(sb)->fs_generation) { >>>>> + di->i_fs_generation = cpu_to_le32(OCFS2_SB(sb)->fs_generation); >>>>> + changed = 1; >>>>> + mlog(ML_ERROR, >>>>> + "Filecheck: reset dinode #%llu: fs_generation to %u\n", >>>>> + (unsigned long long)bh->b_blocknr, >>>>> + le32_to_cpu(di->i_fs_generation)); >>>>> + } >>>>> + >>>>> + if (changed || >>>>> + ocfs2_validate_meta_ecc(sb, bh->b_data, &di->i_check)) { >>>>> + ocfs2_compute_meta_ecc(sb, bh->b_data, &di->i_check); >>>>> + mark_buffer_dirty(bh); >>>>> + mlog(ML_ERROR, >>>>> + "Filecheck: reset dinode #%llu: compute meta ecc\n", >>>>> + (unsigned long long)bh->b_blocknr); >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>
>>> > On 11/03/2015 04:47 PM, Gang He wrote: >> >> >> >>>>> >>> On 11/03/2015 04:15 PM, Gang He wrote: >>>> Hello Junxiao, >>>> >>>> See my comments inline. >>>> >>>> >>>>>>> >>>>> Hi Gang, >>>>> >>>>> This is not like a right patch. >>>>> First, online file check only checks inode's block number, valid flag, >>>>> fs generation value, and meta ecc. I never see a real corruption >>>>> happened only on this field, if these fields are corrupted, that means >>>>> something bad may happen on other place. So fix this field may not help >>>>> and even cause corruption more hard. >>>> This online file check/fix feature is used to check/fix some light file meta > >>> block corruption, instead of turning a file system off and using fsck.ocfs2. >>> What's light meta block corruption? Do you have a case about it? >>>> e.g. meta ecc error, we really need not to use fsck.ocfs2. >>>> of course, this feature does not replace fsck.ocfs2 and touch some >>> complicated meta block problems, if there is some potential problem in some >>> areas, we can discuss them one by one. >>>> >>>> >>>> >>>>> Second, the repair way is wrong. In >>>>> ocfs2_filecheck_repair_inode_block(), if these fields in disk don't >>>>> match the ones in memory, the ones in memory are used to update the disk >>>>> fields. The question is how do you know these field in memory are >>>>> right(they may be the real corrupted ones)? >>>> Here, if the inode block was corrupted, the file system is not able to load >>> it into the memory. >>> How do you know inode block corrupted? If bh for inode block is >>> overwritten, i mean bh corrupted, the repair will corrupted a good inode >>> block. >> You know, the meta block is only validated when the file system loads the > block from disk to memory. >> If the inode object is in the memory, we consider this inode block is OK. > This assuming is not true as there are always bugs. Bugs can make inode > object in memory bad and corrupted the fs when repair the inode. The inode object in the memory has probably been corrupted in some cases, right. but in these cases, online file check/fix feature considers this inode object is validated, will not do any further modification and exit. if the next corruption happens in case this inode is flushed into the disk, the bad thing is not made by online file check/fix code. Next (maybe next mount), the inode block is reload into the memory by the file system will fail, with a kernel error log printing, the online file check/fix probably can help to fix at this monent. Thanks Gang > > Thanks, > Junxiao. >> If the inode is not loaded by the file system via the normal way, the file > system will print a kernel error log to tell which ino is corrupted. >> we will use ocfs2_filecheck_repair_inode_block() function to fix the inode > block before loading. >> >> Thanks >> Gang >> >>> >>> Thanks, >>> Junxiao. >>> >>>> ocfs2_filecheck_repair_inode_block() will able to load it into the memory, >>> since it try to fix these light-level problem before loading. >>>> if the fix is OK, the changed meta-block can pass the block-validate function >>> and load into the memory as a inode object. >>>> Since the file system is under a cluster environment, we have to use some >>> existing function and code path to keep these block operation under a > cluster >>> lock. >>>> >>>> >>>> Thanks >>>> Gang >>>> >>>>> >>>>> Thanks, >>>>> Junxiao. >>>>> On 10/28/2015 02:26 PM, Gang He wrote: >>>>>> +static int ocfs2_filecheck_repair_inode_block(struct super_block *sb, >>>>>> + struct buffer_head *bh) >>>>>> +{ >>>>>> + int rc; >>>>>> + int changed = 0; >>>>>> + struct ocfs2_dinode *di = (struct ocfs2_dinode *)bh->b_data; >>>>>> + >>>>>> + rc = ocfs2_filecheck_validate_inode_block(sb, bh); >>>>>> + /* Can't fix invalid inode block */ >>>>>> + if (!rc || rc == -OCFS2_FILECHECK_ERR_INVALIDINO) >>>>>> + return rc; >>>>>> + >>>>>> + trace_ocfs2_filecheck_repair_inode_block( >>>>>> + (unsigned long long)bh->b_blocknr); >>>>>> + >>>>>> + if (ocfs2_is_hard_readonly(OCFS2_SB(sb)) || >>>>>> + ocfs2_is_soft_readonly(OCFS2_SB(sb))) { >>>>>> + mlog(ML_ERROR, >>>>>> + "Filecheck: try to repair dinode #%llu on readonly filesystem\n", >>>>>> + (unsigned long long)bh->b_blocknr); >>>>>> + return -OCFS2_FILECHECK_ERR_READONLY; >>>>>> + } >>>>>> + >>>>>> + if (le64_to_cpu(di->i_blkno) != bh->b_blocknr) { >>>>>> + di->i_blkno = cpu_to_le64(bh->b_blocknr); >>>>>> + changed = 1; >>>>>> + mlog(ML_ERROR, >>>>>> + "Filecheck: reset dinode #%llu: i_blkno to %llu\n", >>>>>> + (unsigned long long)bh->b_blocknr, >>>>>> + (unsigned long long)le64_to_cpu(di->i_blkno)); >>>>>> + } >>>>>> + >>>>>> + if (!(di->i_flags & cpu_to_le32(OCFS2_VALID_FL))) { >>>>>> + di->i_flags |= cpu_to_le32(OCFS2_VALID_FL); >>>>>> + changed = 1; >>>>>> + mlog(ML_ERROR, >>>>>> + "Filecheck: reset dinode #%llu: OCFS2_VALID_FL is set\n", >>>>>> + (unsigned long long)bh->b_blocknr); >>>>>> + } >>>>>> + >>>>>> + if (le32_to_cpu(di->i_fs_generation) != >>>>>> + OCFS2_SB(sb)->fs_generation) { >>>>>> + di->i_fs_generation = cpu_to_le32(OCFS2_SB(sb)->fs_generation); >>>>>> + changed = 1; >>>>>> + mlog(ML_ERROR, >>>>>> + "Filecheck: reset dinode #%llu: fs_generation to %u\n", >>>>>> + (unsigned long long)bh->b_blocknr, >>>>>> + le32_to_cpu(di->i_fs_generation)); >>>>>> + } >>>>>> + >>>>>> + if (changed || >>>>>> + ocfs2_validate_meta_ecc(sb, bh->b_data, &di->i_check)) { >>>>>> + ocfs2_compute_meta_ecc(sb, bh->b_data, &di->i_check); >>>>>> + mark_buffer_dirty(bh); >>>>>> + mlog(ML_ERROR, >>>>>> + "Filecheck: reset dinode #%llu: compute meta ecc\n", >>>>>> + (unsigned long long)bh->b_blocknr); >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>
Hi Junxiao, On Tue, Nov 03, 2015 at 03:12:35PM +0800, Junxiao Bi wrote: > Hi Gang, > > This is not like a right patch. > First, online file check only checks inode's block number, valid flag, > fs generation value, and meta ecc. I never see a real corruption > happened only on this field, if these fields are corrupted, that means > something bad may happen on other place. So fix this field may not help > and even cause corruption more hard. I agree that these are rather uncommon, we might even consider removing the VALID_FL fixup. I definitely don't think we're ready for anything more complicated than this though either. We kind of have to start somewhere too. > Second, the repair way is wrong. In > ocfs2_filecheck_repair_inode_block(), if these fields in disk don't > match the ones in memory, the ones in memory are used to update the disk > fields. The question is how do you know these field in memory are > right(they may be the real corrupted ones)? Your second point (and the last part of your 1st point) makes a good argument for why this shouldn't happen automatically. Some of these corruptions might require a human to look at the log and decide what to do. Especially as you point out, where we might not know where the source of the corruption is. And if the human can't figure it out, then it's probably time to unmount and fsck. Thanks, --Mark -- Mark Fasheh
Hi Mark, On 11/25/2015 06:16 AM, Mark Fasheh wrote: > Hi Junxiao, > > On Tue, Nov 03, 2015 at 03:12:35PM +0800, Junxiao Bi wrote: >> Hi Gang, >> >> This is not like a right patch. >> First, online file check only checks inode's block number, valid flag, >> fs generation value, and meta ecc. I never see a real corruption >> happened only on this field, if these fields are corrupted, that means >> something bad may happen on other place. So fix this field may not help >> and even cause corruption more hard. > > I agree that these are rather uncommon, we might even consider removing the > VALID_FL fixup. I definitely don't think we're ready for anything more > complicated than this though either. We kind of have to start somewhere too. > Yes, the fix is too simple, and just a start, I think we'd better wait more useful parts done before merging it. > >> Second, the repair way is wrong. In >> ocfs2_filecheck_repair_inode_block(), if these fields in disk don't >> match the ones in memory, the ones in memory are used to update the disk >> fields. The question is how do you know these field in memory are >> right(they may be the real corrupted ones)? > > Your second point (and the last part of your 1st point) makes a good > argument for why this shouldn't happen automatically. Some of these > corruptions might require a human to look at the log and decide what to do. > Especially as you point out, where we might not know where the source of the > corruption is. And if the human can't figure it out, then it's probably time > to unmount and fsck. The point is that the fix way is wrong, just flush memory info to disk is not right. I agree online fsck is good feature, but need carefully design, it should not involve more corruptions. A rough idea from mine is that maybe we need some "frezee" mechanism in fs, which can hung all fs op and let fs stop at a safe area. After freeze fs, we can do some fsck work on it and these works should not cost lots time. What's your idea? Thanks, Junxiao. > > Thanks, > --Mark > > -- > Mark Fasheh >
Hi Mark and Junxiao, >>> > Hi Mark, > > On 11/25/2015 06:16 AM, Mark Fasheh wrote: >> Hi Junxiao, >> >> On Tue, Nov 03, 2015 at 03:12:35PM +0800, Junxiao Bi wrote: >>> Hi Gang, >>> >>> This is not like a right patch. >>> First, online file check only checks inode's block number, valid flag, >>> fs generation value, and meta ecc. I never see a real corruption >>> happened only on this field, if these fields are corrupted, that means >>> something bad may happen on other place. So fix this field may not help >>> and even cause corruption more hard. >> >> I agree that these are rather uncommon, we might even consider removing the >> VALID_FL fixup. I definitely don't think we're ready for anything more >> complicated than this though either. We kind of have to start somewhere too. >> > Yes, the fix is too simple, and just a start, I think we'd better wait > more useful parts done before merging it. I agree, just remark VALID_FL flag to fix this field is too simple, we should delay this field fix before I have a flawless solution, I will remove these lines code in the first version patches. In the future submits, I also hope your guys to help review the code carefully, shout out your comments when you doubt somewhere. >> >>> Second, the repair way is wrong. In >>> ocfs2_filecheck_repair_inode_block(), if these fields in disk don't >>> match the ones in memory, the ones in memory are used to update the disk >>> fields. The question is how do you know these field in memory are >>> right(they may be the real corrupted ones)? >> >> Your second point (and the last part of your 1st point) makes a good >> argument for why this shouldn't happen automatically. Some of these >> corruptions might require a human to look at the log and decide what to do. >> Especially as you point out, where we might not know where the source of the >> corruption is. And if the human can't figure it out, then it's probably time >> to unmount and fsck. > The point is that the fix way is wrong, just flush memory info to disk > is not right. I agree online fsck is good feature, but need carefully > design, it should not involve more corruptions. A rough idea from mine > is that maybe we need some "frezee" mechanism in fs, which can hung all > fs op and let fs stop at a safe area. After freeze fs, we can do some > fsck work on it and these works should not cost lots time. What's your idea? If we need to touch some global data structures, freezing fs can be considered when we can't get any way in case using the locks. If we only handle some independent problem, we just need to lock the related data structures. > > Thanks, > Junxiao. > >> >> Thanks, >> --Mark >> >> -- >> Mark Fasheh >>
On 11/25/2015 01:04 PM, Gang He wrote: > Hi Mark and Junxiao, > > >>>> >> Hi Mark, >> >> On 11/25/2015 06:16 AM, Mark Fasheh wrote: >>> Hi Junxiao, >>> >>> On Tue, Nov 03, 2015 at 03:12:35PM +0800, Junxiao Bi wrote: >>>> Hi Gang, >>>> >>>> This is not like a right patch. >>>> First, online file check only checks inode's block number, valid flag, >>>> fs generation value, and meta ecc. I never see a real corruption >>>> happened only on this field, if these fields are corrupted, that means >>>> something bad may happen on other place. So fix this field may not help >>>> and even cause corruption more hard. >>> >>> I agree that these are rather uncommon, we might even consider removing the >>> VALID_FL fixup. I definitely don't think we're ready for anything more >>> complicated than this though either. We kind of have to start somewhere too. >>> >> Yes, the fix is too simple, and just a start, I think we'd better wait >> more useful parts done before merging it. > I agree, just remark VALID_FL flag to fix this field is too simple, we should delay this field fix before > I have a flawless solution, I will remove these lines code in the first version patches. In the future submits, > I also hope your guys to help review the code carefully, shout out your comments when you doubt somewhere. Sure. > > > >>> >>>> Second, the repair way is wrong. In >>>> ocfs2_filecheck_repair_inode_block(), if these fields in disk don't >>>> match the ones in memory, the ones in memory are used to update the disk >>>> fields. The question is how do you know these field in memory are >>>> right(they may be the real corrupted ones)? >>> >>> Your second point (and the last part of your 1st point) makes a good >>> argument for why this shouldn't happen automatically. Some of these >>> corruptions might require a human to look at the log and decide what to do. >>> Especially as you point out, where we might not know where the source of the >>> corruption is. And if the human can't figure it out, then it's probably time >>> to unmount and fsck. >> The point is that the fix way is wrong, just flush memory info to disk >> is not right. I agree online fsck is good feature, but need carefully >> design, it should not involve more corruptions. A rough idea from mine >> is that maybe we need some "frezee" mechanism in fs, which can hung all >> fs op and let fs stop at a safe area. After freeze fs, we can do some >> fsck work on it and these works should not cost lots time. What's your idea? > If we need to touch some global data structures, freezing fs can be considered when we can't > get any way in case using the locks. > If we only handle some independent problem, we just need to lock the related data structures. Hmm, I am not sure whether it's hard to decide an independent issue. Thanks, Junxiao. > >> >> Thanks, >> Junxiao. >> >>> >>> Thanks, >>> --Mark >>> >>> -- >>> Mark Fasheh >>> >
diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c index b254416..d811698 100644 --- a/fs/ocfs2/inode.c +++ b/fs/ocfs2/inode.c @@ -53,6 +53,7 @@ #include "xattr.h" #include "refcounttree.h" #include "ocfs2_trace.h" +#include "filecheck.h" #include "buffer_head_io.h" @@ -74,6 +75,13 @@ static int ocfs2_truncate_for_delete(struct ocfs2_super *osb, struct inode *inode, struct buffer_head *fe_bh); +static int ocfs2_filecheck_read_inode_block_full(struct inode *inode, + struct buffer_head **bh, int flags, int type); +static int ocfs2_filecheck_validate_inode_block(struct super_block *sb, + struct buffer_head *bh); +static int ocfs2_filecheck_repair_inode_block(struct super_block *sb, + struct buffer_head *bh); + void ocfs2_set_inode_flags(struct inode *inode) { unsigned int flags = OCFS2_I(inode)->ip_attr; @@ -127,6 +135,7 @@ struct inode *ocfs2_ilookup(struct super_block *sb, u64 blkno) struct inode *ocfs2_iget(struct ocfs2_super *osb, u64 blkno, unsigned flags, int sysfile_type) { + int rc = 0; struct inode *inode = NULL; struct super_block *sb = osb->sb; struct ocfs2_find_inode_args args; @@ -161,12 +170,17 @@ struct inode *ocfs2_iget(struct ocfs2_super *osb, u64 blkno, unsigned flags, } trace_ocfs2_iget5_locked(inode->i_state); if (inode->i_state & I_NEW) { - ocfs2_read_locked_inode(inode, &args); + rc = ocfs2_read_locked_inode(inode, &args); unlock_new_inode(inode); } if (is_bad_inode(inode)) { iput(inode); - inode = ERR_PTR(-ESTALE); + if ((flags & OCFS2_FI_FLAG_FILECHECK_CHK) || + (flags & OCFS2_FI_FLAG_FILECHECK_FIX)) + /* Return OCFS2_FILECHECK_ERR_XXX related errno */ + inode = ERR_PTR(rc); + else + inode = ERR_PTR(-ESTALE); goto bail; } @@ -494,16 +508,32 @@ static int ocfs2_read_locked_inode(struct inode *inode, } if (can_lock) { - status = ocfs2_read_inode_block_full(inode, &bh, - OCFS2_BH_IGNORE_CACHE); + if (args->fi_flags & OCFS2_FI_FLAG_FILECHECK_CHK) + status = ocfs2_filecheck_read_inode_block_full(inode, + &bh, OCFS2_BH_IGNORE_CACHE, 0); + else if (args->fi_flags & OCFS2_FI_FLAG_FILECHECK_FIX) + status = ocfs2_filecheck_read_inode_block_full(inode, + &bh, OCFS2_BH_IGNORE_CACHE, 1); + else + status = ocfs2_read_inode_block_full(inode, + &bh, OCFS2_BH_IGNORE_CACHE); } else { status = ocfs2_read_blocks_sync(osb, args->fi_blkno, 1, &bh); /* * If buffer is in jbd, then its checksum may not have been * computed as yet. */ - if (!status && !buffer_jbd(bh)) - status = ocfs2_validate_inode_block(osb->sb, bh); + if (!status && !buffer_jbd(bh)) { + if (args->fi_flags & OCFS2_FI_FLAG_FILECHECK_CHK) + status = ocfs2_filecheck_validate_inode_block( + osb->sb, bh); + else if (args->fi_flags & OCFS2_FI_FLAG_FILECHECK_FIX) + status = ocfs2_filecheck_repair_inode_block( + osb->sb, bh); + else + status = ocfs2_validate_inode_block( + osb->sb, bh); + } } if (status < 0) { mlog_errno(status); @@ -531,6 +561,14 @@ static int ocfs2_read_locked_inode(struct inode *inode, BUG_ON(args->fi_blkno != le64_to_cpu(fe->i_blkno)); + if (buffer_dirty(bh)) { + status = ocfs2_write_block(osb, bh, INODE_CACHE(inode)); + if (status < 0) { + mlog_errno(status); + goto bail; + } + } + status = 0; bail: @@ -1385,6 +1423,152 @@ bail: return rc; } +static int ocfs2_filecheck_validate_inode_block(struct super_block *sb, + struct buffer_head *bh) +{ + int rc = 0; + struct ocfs2_dinode *di = (struct ocfs2_dinode *)bh->b_data; + + trace_ocfs2_filecheck_validate_inode_block( + (unsigned long long)bh->b_blocknr); + + BUG_ON(!buffer_uptodate(bh)); + + if (!OCFS2_IS_VALID_DINODE(di)) { + mlog(ML_ERROR, + "Filecheck: invalid dinode #%llu: signature = %.*s\n", + (unsigned long long)bh->b_blocknr, 7, di->i_signature); + rc = -OCFS2_FILECHECK_ERR_INVALIDINO; + goto bail; + } + + rc = ocfs2_validate_meta_ecc(sb, bh->b_data, &di->i_check); + if (rc) { + mlog(ML_ERROR, + "Filecheck: checksum failed for dinode %llu\n", + (unsigned long long)bh->b_blocknr); + rc = -OCFS2_FILECHECK_ERR_BLOCKECC; + goto bail; + } + + if (le64_to_cpu(di->i_blkno) != bh->b_blocknr) { + mlog(ML_ERROR, + "Filecheck: invalid dinode #%llu: i_blkno is %llu\n", + (unsigned long long)bh->b_blocknr, + (unsigned long long)le64_to_cpu(di->i_blkno)); + rc = -OCFS2_FILECHECK_ERR_BLOCKNO; + goto bail; + } + + if (!(di->i_flags & cpu_to_le32(OCFS2_VALID_FL))) { + mlog(ML_ERROR, + "Filecheck: invalid dinode #%llu: OCFS2_VALID_FL not set\n", + (unsigned long long)bh->b_blocknr); + rc = -OCFS2_FILECHECK_ERR_VALIDFLAG; + goto bail; + } + + if (le32_to_cpu(di->i_fs_generation) != + OCFS2_SB(sb)->fs_generation) { + mlog(ML_ERROR, + "Filecheck: invalid dinode #%llu: fs_generation is %u\n", + (unsigned long long)bh->b_blocknr, + le32_to_cpu(di->i_fs_generation)); + rc = -OCFS2_FILECHECK_ERR_GENERATION; + goto bail; + } + +bail: + return rc; +} + +static int ocfs2_filecheck_repair_inode_block(struct super_block *sb, + struct buffer_head *bh) +{ + int rc; + int changed = 0; + struct ocfs2_dinode *di = (struct ocfs2_dinode *)bh->b_data; + + rc = ocfs2_filecheck_validate_inode_block(sb, bh); + /* Can't fix invalid inode block */ + if (!rc || rc == -OCFS2_FILECHECK_ERR_INVALIDINO) + return rc; + + trace_ocfs2_filecheck_repair_inode_block( + (unsigned long long)bh->b_blocknr); + + if (ocfs2_is_hard_readonly(OCFS2_SB(sb)) || + ocfs2_is_soft_readonly(OCFS2_SB(sb))) { + mlog(ML_ERROR, + "Filecheck: try to repair dinode #%llu on readonly filesystem\n", + (unsigned long long)bh->b_blocknr); + return -OCFS2_FILECHECK_ERR_READONLY; + } + + if (le64_to_cpu(di->i_blkno) != bh->b_blocknr) { + di->i_blkno = cpu_to_le64(bh->b_blocknr); + changed = 1; + mlog(ML_ERROR, + "Filecheck: reset dinode #%llu: i_blkno to %llu\n", + (unsigned long long)bh->b_blocknr, + (unsigned long long)le64_to_cpu(di->i_blkno)); + } + + if (!(di->i_flags & cpu_to_le32(OCFS2_VALID_FL))) { + di->i_flags |= cpu_to_le32(OCFS2_VALID_FL); + changed = 1; + mlog(ML_ERROR, + "Filecheck: reset dinode #%llu: OCFS2_VALID_FL is set\n", + (unsigned long long)bh->b_blocknr); + } + + if (le32_to_cpu(di->i_fs_generation) != + OCFS2_SB(sb)->fs_generation) { + di->i_fs_generation = cpu_to_le32(OCFS2_SB(sb)->fs_generation); + changed = 1; + mlog(ML_ERROR, + "Filecheck: reset dinode #%llu: fs_generation to %u\n", + (unsigned long long)bh->b_blocknr, + le32_to_cpu(di->i_fs_generation)); + } + + if (changed || + ocfs2_validate_meta_ecc(sb, bh->b_data, &di->i_check)) { + ocfs2_compute_meta_ecc(sb, bh->b_data, &di->i_check); + mark_buffer_dirty(bh); + mlog(ML_ERROR, + "Filecheck: reset dinode #%llu: compute meta ecc\n", + (unsigned long long)bh->b_blocknr); + } + + return 0; +} + +static int +ocfs2_filecheck_read_inode_block_full(struct inode *inode, + struct buffer_head **bh, int flags, int type) +{ + int rc; + struct buffer_head *tmp = *bh; + + if (!type) /* Check inode block */ + rc = ocfs2_read_blocks(INODE_CACHE(inode), + OCFS2_I(inode)->ip_blkno, + 1, &tmp, flags, + ocfs2_filecheck_validate_inode_block); + else /* Repair inode block */ + rc = ocfs2_read_blocks(INODE_CACHE(inode), + OCFS2_I(inode)->ip_blkno, + 1, &tmp, flags, + ocfs2_filecheck_repair_inode_block); + + /* If ocfs2_read_blocks() got us a new bh, pass it up. */ + if (!rc && !*bh) + *bh = tmp; + + return rc; +} + int ocfs2_read_inode_block_full(struct inode *inode, struct buffer_head **bh, int flags) { diff --git a/fs/ocfs2/ocfs2_trace.h b/fs/ocfs2/ocfs2_trace.h index 6cb019b..d9205e0 100644 --- a/fs/ocfs2/ocfs2_trace.h +++ b/fs/ocfs2/ocfs2_trace.h @@ -1540,6 +1540,8 @@ DEFINE_OCFS2_ULL_INT_EVENT(ocfs2_read_locked_inode); DEFINE_OCFS2_INT_INT_EVENT(ocfs2_check_orphan_recovery_state); DEFINE_OCFS2_ULL_EVENT(ocfs2_validate_inode_block); +DEFINE_OCFS2_ULL_EVENT(ocfs2_filecheck_validate_inode_block); +DEFINE_OCFS2_ULL_EVENT(ocfs2_filecheck_repair_inode_block); TRACE_EVENT(ocfs2_inode_is_valid_to_delete, TP_PROTO(void *task, void *dc_task, unsigned long long ino,
Implement online check or fix inode block during reading a inode block to memory. Signed-off-by: Gang He <ghe@suse.com> --- fs/ocfs2/inode.c | 196 +++++++++++++++++++++++++++++++++++++++++++++++-- fs/ocfs2/ocfs2_trace.h | 2 + 2 files changed, 192 insertions(+), 6 deletions(-)