Hi, Eddie, I totally agree with Coly's suggestion about how to use the two new patches in your environment. It will be very helpful to us if you try them. Coly, Thank you very much for sorting out the ins and outs about the patches, the explanation is very helpful and necessary. Thanks. 2017-11-24 2:08 GMT+08:00 Eddie Chapman <eddie@xxxxxxxx>: > Hi Coly, > > > On 23/11/17 16:31, Coly Li wrote: >> >> On 23/11/2017 11:09 PM, Eddie Chapman wrote: >>> >>> On 22/11/17 05:13, Michael Lyle wrote: >>>> >>>> Reviewed-by: Michael Lyle <mlyle@xxxxxxxx> >>> >>> >>> <snip> >>> >>> I have a little dilemma now related to this patch and I wonder if I >>> could ask you guys for your opinion? >>> >> >> Hi Eddie, >> >> As author of "only permit to recovery read error" patch, let me try to >> response your question. >> >>> I have been hit and am being hit by the bug this patch fixes every few >>> days at the moment, on writeback bcache resources. When it hits it >>> causes a failure in the layer above, and this leads to other problems >>> that can and has led to loss of data (not in bcache, just that the read >>> failure causes a chain of events in upper layer, separate issue, won't >>> clutter this mail with that). So, I am very keen to resolve this >>> urgently in my environment. >>> >>> I am running stable 4.9, and I have cherry picked several recent bcache >>> patches, including "bcache: only permit to recovery read error >>> when cache device is clean", which I understand causes this issue. >>> >>> I have a dilemma now whether I should apply the new patch in this >>> thread, which should fix the issue, or revert "only permit to recovery >>> read error" patch. >>> >> >> I suggest you to take one of the 2 actions, >> - apply "bcache: recover data from backing device when read request hit >> clean" patch, or >> - revert "only permit to recovery read error" patch, and add the >> modification as "bcache: recover data from backing device when read >> request hit clean" does. > > > Thanks for clarifying what the 2 best options are. > >>> The dilemma is because I don't like the idea of reverting the "only >>> permit to recovery read error" patch since it appears to fix an >>> important problem. Or am I overestimating the problem (maybe that's why >>> the original patch has not been backported to stable kernels, maybe the >>> original issue quite rare)? >>> >> >> "only permit to recovery read error" patch fixes a real data corruption >> issue with broken cache device (SSD), which was reported from production >> environment. If you don't have all these 2 patches, your data may be >> corrupted in silence when SSD is broken. > > > OK, I think that confirms to me then that I'm definitely keeping the "only > permit to recovery read error" patch :-) > > That sounds like a very significant bug fix. I'm wondering, shouldn't "only > permit to recovery read error" patch be backported to the stable kernels > like 4.9, 4.4? Or maybe it doesn't fully meet the stable inclusion criteria. > I know the patch was CC'd to stable but it has not been picked up. Maybe > because it was not in Linus' tree yet when it was originally CC'd. > >>> But I am also wary of applying the new patch as Michael only just added >>> to his tree so hasn't had much baking time. >>> >> >> The new one "bcache: recover data from backing device when read request >> hit clean" can be treated as an improvement of my patch. Because when I >> wrote that patch, I didn't notice metadata and data failure on cache >> device were in different code path, I have to only check ds->has_dirty. > > > OK, thanks, that makes me feel a little better about applying it. > >> Rui points out the metadata failure is in another code pathm and buggy >> too. He posts another patch to fix it, and you may find it in >> linux-bcache mailing list. > > > Yes, I noticed that patch and was thinking whether to apply that one too. > >> When I wrote the "only permit to recovery read error" patch, I didn't >> notice the read race existed neither, until Junhui and Rui noticed me. >> The read race only happens on clean data (because its bucket is clean >> and re-allocated to different write request), so the new patch permits >> data recovery from backing device if a read failed on clean data. >> >> The result for this new fix is, less I/O error may returns to upper >> layer code, especially the bogus I/O error generated by btree read race. > > > Thanks for explaining and the detail, it is much appreciated :-) And I hope > I don't come across as complaining about this patch in any way :-) I know > this is difficult work, and, hey, even with the new bug introduced, the new > bug is a *lot* less painful than the original bug you fixed :-) > >>> I wondered what is the opinion of you guys which of these two I should >>> choose? >> >> >> If you only do patching, you need to have both of them. If you only want >> one patch, you may rebase this patch to 4.9 kernel and ignore the first >> one. >> >>> >>> (BTW, please don't say "just don't cherry pick onto 4.9". I am aware of >>> the risks of cherry picking, as discussed recently on this list. I am >>> happy with the level of risk and I am being very careful in what I pick. >>> In this case I have not been "burnt" by cherry picking since I would >>> have hit the same issue if I would have just upgraded to 4.14 which >>> contains the original "only permit to recovery read error" patch.) >>> >>> ( N.B. I understand that any opinion/advice I receive is just that. I >>> take complete responsibility for what I decide :-) ) >> >> >> One more comment, when you apply both patches, I/O failure on broken >> cache device (SSD) is still risky. As Rui points out in his patch, >> "The read request might meet error when searching the btree, but the >> error was not handled in cache_lookup(), and this kind of metadata >> failure will not go into cached_dev_read_error(), finally, the upper >> layer will receive bi_status=0." >> >> Rui's patch "bcache: return IOERR to upper layer when read request meet >> metadata error" is also added inti Mike's tree, if you may consider >> testing it, that will be very helpful to us. > > > You have convinced me that the best way forward is to keep "only permit to > recovery read error", and also apply both of the new patches. I think I > will wait a few more days to give a little more "baking" time for the new > patches, then will try them and report back. > >> >> Thank you for asking great question :-) >> > > Thanks! > Eddie -- To unsubscribe from this list: send the line "unsubscribe linux-bcache" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
