linux-kernel - Re: [PATCH] BTRFS: Runs the xor function if a Block has failed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160105050327.1ff85a20@gmail.com>
Date:	Tue, 5 Jan 2016 05:03:27 -0500
From:	Sanidhya Solanki <jpage.lkml@...il.com>
To:	David Sterba <dsterba@...e.cz>
Cc:	clm@...com, jbacik@...com, dsterba@...e.com,
	linux-kernel@...r.kernel.org, linux-btrfs@...r.kernel.org
Subject: Re: [PATCH] BTRFS: Runs the xor function if a Block has failed

On Tue, 5 Jan 2016 10:22:36 +0100
David Sterba <dsterba@...e.cz> wrote:

> If the data a rerecovered, why is -EIO still returned? 

In the other places in the file where the code appears, the submitted
patch is all that is required to do the xor. I think we also need to
include the following line:
memcpy(pointers[nr_data], pointers[0], PAGE_SIZE);
and delete the "return EIO" statement which I believe appears to be a
placeholder for the xor function.
In the end the final patch should look something like this:
> -					 * TODO, we should redo the xor here.
>  					 */
> +					memcpy(pointers[nr_data], pointers[0], PAGE_SIZE);
> +					run_xor(pointers, rbio->nr_data - 1, PAGE_CACHE_SIZE); 
> - err = -EIO;

So, I was just going to send you the mail as it is written above, but I
decided to investigate. The commit in question that added the todo was
53b381b3abeb86f12787a6c40fee9b2f71edc23b. Unfortunately, it was not
submitted  by the original author, nor was it by any means a small
dedicated patch. It adds the entire file, without much comment or
explanation and has not been touched since. 

However, we can get some idea of what is expected by looking at line
2398 in raid56.c, where a similar case of raid 5 recovery is handled.

So what the patch described above does is deal with a scenario where
no q stripe or bad data block exists, and, we can only rebuild from the
p-stripe, in effect like a raid 5 recovery.

So, if you are satisfied with the above retouched change, I can modify
my original patch with your suggestions and my changes, and, I can
forward them to the list again.

> Also, I see some post-recovery steps eg. for the damaged P stripes
> (at label pstripes) and I'd expect something similar for the case
> you're fixing.

I believe that is the case because the other cases still have the q-
stripe available tor rebuild from, which requires cleanup afterwards,
but the raid5-like scenario above does not. Let me know if anything
else is needed.

> I'm not familiar with the raid56 implementation but the fix looks
> suspiciously trivial and I doubt that the xor was omitted out of
> laziness.

I guess we will never know.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/