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
