Ocfs2 devel - Feb 2016 - [PATCH v3 4/4] ocfs2: check/fix inode block for online file check

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