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.
> 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?
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
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail
