* [Ocfs2-devel] [PATCH v3 4/4] ocfs2: check/fix inode block for online file check
[not found] ` <20160205230533.GB1239@wotan.suse.de>
@ 2016-02-06 1:02 ` Gang He
0 siblings, 0 replies; 6+ messages in thread
From: Gang He @ 2016-02-06 1:02 UTC (permalink / raw)
To: ocfs2-devel
Hello Mark,
Thanks for your comments.
-Gang
>>>
> On Fri, Feb 05, 2016 at 06:35:07AM -0700, Gang He wrote:
>> Hi Mark,
>>
>>
>> >>>
>> > On Sun, Jan 24, 2016 at 11:11:33PM -0700, Gang He wrote:
>> >> >> Also, I'm concerned that the buffer in question might be journaled. In
> that
>> >
>> >> >> case, writing it to disk like this could cause corruptions (if the buffer
>> >> >> contains not-committed changes).
>> >> > I ever though of journaling this changed inode block in case file check
>> >> > fixing, but you know, we are being on inode block loading stage, the
>> > journal
>> >> > related structs are not prepared at this moment, then I write this block
>> > back
>> >> > to the disk synchronously within ocfs2_inode_lock, it looks a little
>> > tricky,
>> >> > but not bring any risk. in case the machine crashes when writing the inode
>
>> >> > block back to the disk, this will not affect file system integrity, since
>> >> > this inode block original is corrupted, the user can fix this inode block
>> > via
>> >> > file check again after the machine is recovered.
>> >> > Anyway, I just want to let all know what I think behind this part code,
>> >> > maybe it is not right, please give your feedback again.
>> >
>> > So the problem is a buffer that might exist and be journaled without the
>> > inode being in
>> > memory. I'm not clear on whether we would hit this (even if we did it would
>> > be rare). It might be easiest if you could test the buffer with
> buffer_jbd()
>> > and warn then return an error to see if we ever even hit that case.
>> Thank for your comments, I can add this test buffer_jbd() in code, to avoid
> such race condition (though I feel there is not possibility).
>> Besides the comments above, do you have any other comments for this part
> code?
>> I will submit a updated patches after Chinese New Year holiday.
>
> No, I think it looks good overall. You should have my reviewed-by for most
> of the patches by now, so it's just this one if I remember?
> --Mark
>
> --
> Mark Fasheh
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Ocfs2-devel] [PATCH v3 4/4] ocfs2: check/fix inode block for online file check
2016-01-14 1:40 ` Mark Fasheh
@ 2016-01-14 6:30 ` Gang He
-1 siblings, 0 replies; 6+ messages in thread
From: Gang He @ 2016-01-14 6:30 UTC (permalink / raw)
To: Mark Fasheh; +Cc: ocfs2-devel, rgoldwyn, linux-kernel
Hello Mark,
>>>
> On Fri, Dec 25, 2015 at 03:16:19PM +0800, Gang He wrote:
>> 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 | 200
> +++++++++++++++++++++++++++++++++++++++++++++++--
>> fs/ocfs2/ocfs2_trace.h | 2 +
>> 2 files changed, 196 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
>> index 8f87e05..6ac2f19 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,14 @@ 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 +136,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 +171,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 +509,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 +562,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;
>> + }
>> + }
>
> This reminds me, we should be checking for a readonly file system up top in
> the 'fix' helper.
When the file system becomes read-only, we still can check certain file via it's inode number and report the result (the inode block is integrated or not),
but we cannot fix this file in file system read-only status, otherwise the subsequent logic will become too complex in case we modify/write a inode block under a read-only file system.
Actually, online file check feature should be combined with "errors=continues" option for better using.
>
> Also, I'm concerned that the buffer in question might be journaled. In that
> case, writing it to disk like this could cause corruptions (if the buffer
> contains not-committed changes).
I ever though of journaling this changed inode block in case file check fixing, but you know, we are being on inode block loading stage, the journal related structs are not prepared at this moment, then I write this block back to the disk synchronously within ocfs2_inode_lock, it looks a little tricky, but not bring any risk. in case the machine crashes when writing the inode block back to the disk, this will not affect file system integrity, since this inode block original is corrupted, the user can fix this inode block via file check again after the machine is recovered.
Anyway, I just want to let all know what I think behind this part code, maybe it is not right, please give your feedback again.
Thanks
Gang
>
> The answer might be to journal the changes we make but I'm not sure if that
> can deadlock with other iget() calls.
> --Mark
>
> --
> Mark Fasheh
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH v3 4/4] ocfs2: check/fix inode block for online file check
@ 2016-01-14 6:30 ` Gang He
0 siblings, 0 replies; 6+ messages in thread
From: Gang He @ 2016-01-14 6:30 UTC (permalink / raw)
To: Mark Fasheh; +Cc: ocfs2-devel, rgoldwyn, linux-kernel
Hello Mark,
>>>
> On Fri, Dec 25, 2015 at 03:16:19PM +0800, Gang He wrote:
>> 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 | 200
> +++++++++++++++++++++++++++++++++++++++++++++++--
>> fs/ocfs2/ocfs2_trace.h | 2 +
>> 2 files changed, 196 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
>> index 8f87e05..6ac2f19 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,14 @@ 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 +136,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 +171,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 +509,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 +562,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;
>> + }
>> + }
>
> This reminds me, we should be checking for a readonly file system up top in
> the 'fix' helper.
When the file system becomes read-only, we still can check certain file via it's inode number and report the result (the inode block is integrated or not),
but we cannot fix this file in file system read-only status, otherwise the subsequent logic will become too complex in case we modify/write a inode block under a read-only file system.
Actually, online file check feature should be combined with "errors=continues" option for better using.
>
> Also, I'm concerned that the buffer in question might be journaled. In that
> case, writing it to disk like this could cause corruptions (if the buffer
> contains not-committed changes).
I ever though of journaling this changed inode block in case file check fixing, but you know, we are being on inode block loading stage, the journal related structs are not prepared at this moment, then I write this block back to the disk synchronously within ocfs2_inode_lock, it looks a little tricky, but not bring any risk. in case the machine crashes when writing the inode block back to the disk, this will not affect file system integrity, since this inode block original is corrupted, the user can fix this inode block via file check again after the machine is recovered.
Anyway, I just want to let all know what I think behind this part code, maybe it is not right, please give your feedback again.
Thanks
Gang
>
> The answer might be to journal the changes we make but I'm not sure if that
> can deadlock with other iget() calls.
> --Mark
>
> --
> Mark Fasheh
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Ocfs2-devel] [PATCH v3 4/4] ocfs2: check/fix inode block for online file check
2015-12-25 7:16 ` [Ocfs2-devel] [PATCH v3 4/4] ocfs2: check/fix inode block for online file check Gang He
@ 2016-01-14 1:40 ` Mark Fasheh
0 siblings, 0 replies; 6+ messages in thread
From: Mark Fasheh @ 2016-01-14 1:40 UTC (permalink / raw)
To: Gang He; +Cc: rgoldwyn, linux-kernel, ocfs2-devel
On Fri, Dec 25, 2015 at 03:16:19PM +0800, Gang He wrote:
> 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 | 200 +++++++++++++++++++++++++++++++++++++++++++++++--
> fs/ocfs2/ocfs2_trace.h | 2 +
> 2 files changed, 196 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
> index 8f87e05..6ac2f19 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,14 @@ 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 +136,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 +171,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 +509,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 +562,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;
> + }
> + }
This reminds me, we should be checking for a readonly file system up top in
the 'fix' helper.
Also, I'm concerned that the buffer in question might be journaled. In that
case, writing it to disk like this could cause corruptions (if the buffer
contains not-committed changes).
The answer might be to journal the changes we make but I'm not sure if that
can deadlock with other iget() calls.
--Mark
--
Mark Fasheh
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH v3 4/4] ocfs2: check/fix inode block for online file check
@ 2016-01-14 1:40 ` Mark Fasheh
0 siblings, 0 replies; 6+ messages in thread
From: Mark Fasheh @ 2016-01-14 1:40 UTC (permalink / raw)
To: Gang He; +Cc: rgoldwyn, linux-kernel, ocfs2-devel
On Fri, Dec 25, 2015 at 03:16:19PM +0800, Gang He wrote:
> 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 | 200 +++++++++++++++++++++++++++++++++++++++++++++++--
> fs/ocfs2/ocfs2_trace.h | 2 +
> 2 files changed, 196 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
> index 8f87e05..6ac2f19 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,14 @@ 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 +136,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 +171,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 +509,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 +562,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;
> + }
> + }
This reminds me, we should be checking for a readonly file system up top in
the 'fix' helper.
Also, I'm concerned that the buffer in question might be journaled. In that
case, writing it to disk like this could cause corruptions (if the buffer
contains not-committed changes).
The answer might be to journal the changes we make but I'm not sure if that
can deadlock with other iget() calls.
--Mark
--
Mark Fasheh
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH v3 4/4] ocfs2: check/fix inode block for online file check
2015-12-25 7:16 [PATCH v3 0/4] Add online file check feature Gang He
@ 2015-12-25 7:16 ` Gang He
2016-01-14 1:40 ` Mark Fasheh
0 siblings, 1 reply; 6+ messages in thread
From: Gang He @ 2015-12-25 7:16 UTC (permalink / raw)
To: mfasheh, rgoldwyn, ghe; +Cc: linux-kernel, ocfs2-devel, akpm
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 | 200 +++++++++++++++++++++++++++++++++++++++++++++++--
fs/ocfs2/ocfs2_trace.h | 2 +
2 files changed, 196 insertions(+), 6 deletions(-)
diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
index 8f87e05..6ac2f19 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,14 @@ 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 +136,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 +171,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 +509,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 +562,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:
@@ -1396,6 +1435,155 @@ 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));
+
+ 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 (!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;
+ }
+
+ 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 changed = 0;
+ struct ocfs2_dinode *di = (struct ocfs2_dinode *)bh->b_data;
+
+ if (!ocfs2_filecheck_validate_inode_block(sb, bh))
+ return 0;
+
+ 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: cannot repair dinode #%llu "
+ "on readonly filesystem\n",
+ (unsigned long long)bh->b_blocknr);
+ return -OCFS2_FILECHECK_ERR_READONLY;
+ }
+
+ if (!OCFS2_IS_VALID_DINODE(di)) {
+ /* Cannot fix invalid inode block */
+ return -OCFS2_FILECHECK_ERR_INVALIDINO;
+ }
+
+ if (!(di->i_flags & cpu_to_le32(OCFS2_VALID_FL))) {
+ /* Cannot just add VALID_FL flag back as a fix,
+ * need more things to check here.
+ */
+ return -OCFS2_FILECHECK_ERR_VALIDFLAG;
+ }
+
+ 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 (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,
--
2.1.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-02-06 1:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <56A62D15020000F9000270A2@relay2.provo.novell.com>
[not found] ` <20160204004510.GI24672@wotan.suse.de>
[not found] ` <56B5158B020000F900022289@relay2.provo.novell.com>
[not found] ` <20160205230533.GB1239@wotan.suse.de>
2016-02-06 1:02 ` [Ocfs2-devel] [PATCH v3 4/4] ocfs2: check/fix inode block for online file check Gang He
2015-12-25 7:16 [PATCH v3 0/4] Add online file check feature Gang He
2015-12-25 7:16 ` [Ocfs2-devel] [PATCH v3 4/4] ocfs2: check/fix inode block for online file check Gang He
2016-01-14 1:40 ` Mark Fasheh
2016-01-14 1:40 ` Mark Fasheh
2016-01-14 6:30 ` Gang He
2016-01-14 6:30 ` Gang He
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.