Re: [PATCH] ext4: correct error value of function verifying dx checksum

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, May 19, 2016 at 09:40:04PM -0600, Andreas Dilger wrote:
> On May 19, 2016, at 7:51 PM, Darrick J. Wong <darrick.wong@xxxxxxxxxx> wrote:
> > 
> > On Fri, May 20, 2016 at 08:54:56AM +0900, Daeho Jeong wrote:
> >> ext4_dx_csum_verify() returns the success return value in two checksum
> >> verification failure cases. We need to set the return values to zero
> >> as failure like ext4_dirent_csum_verify() returning zero when failing
> >> to find a checksum dirent at the tail.
> 
> It would be useful to add a comment block to this function that describes
> the return values.  Clearly, if the author didn't get the return values
> correct, it seems likely that someone else may be confused in the future.
> The function itself isn't named clearly enough to know whether the return
> of "1" or "0" should be considered an error.  If it were named something
> like "ext4_dx_csum_valid()" then clearly "1" would mean it is valid and
> "0" would mean it is invalid.

<shrug> verify->valid would make the name clearer; maybe the return type
ought to be bool too.

(That said, I think I got them right; it's just my evaluation of what
counts as a soft error and what counts as a hard-error-shut-it-down have
shfited over five years.)

> > ISTR deciding back in 2011 that "can't find the checksums" wasn't a hard enough
> > error to warrant shutting down the FS.  Though, being unable to find the limit
> > and count fields of a dx node /is/ bad enough, I think.
> > 
> > 2016 me is more paranoid about soft errors, so:
> > Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> 
> My recollection is that there are some cases where adding a checksum to an
> existing directory that didn't have enough space for the tail would leave the
> directory with no checksum?  What does e2fsck do in this case when adding
> checksums to an existing directory?  Skip the tail or split the block?

It rebuilds the entire directory. :)

--D

> 
> Cheers, Andreas
> 
> >> Signed-off-by: Daeho Jeong <daeho.jeong@xxxxxxxxxxx>
> >> ---
> >> fs/ext4/namei.c |    4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> >> index 48e4b89..ec811bb 100644
> >> --- a/fs/ext4/namei.c
> >> +++ b/fs/ext4/namei.c
> >> @@ -446,14 +446,14 @@ static int ext4_dx_csum_verify(struct inode *inode,
> >> 	c = get_dx_countlimit(inode, dirent, &count_offset);
> >> 	if (!c) {
> >> 		EXT4_ERROR_INODE(inode, "dir seems corrupt?  Run e2fsck -D.");
> >> -		return 1;
> >> +		return 0;
> >> 	}
> >> 	limit = le16_to_cpu(c->limit);
> >> 	count = le16_to_cpu(c->count);
> >> 	if (count_offset + (limit * sizeof(struct dx_entry)) >
> >> 	    EXT4_BLOCK_SIZE(inode->i_sb) - sizeof(struct dx_tail)) {
> >> 		warn_no_space_for_csum(inode);
> >> -		return 1;
> >> +		return 0;
> >> 	}
> >> 	t = (struct dx_tail *)(((struct dx_entry *)c) + limit);
> >> 
> >> --
> >> 1.7.9.5
> >> 
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> >> the body of a message to majordomo@xxxxxxxxxxxxxxx
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> Cheers, Andreas
> 
> 
> 
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux