<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> Hi, <div class="moz-cite-prefix">On 06/26/2017 11:27 PM, Eric Ren wrote: </div> <blockquote cite="mid:a2dcb2e0-cba2-6776-f041-605a66486ec5@suse.com" type="cite">Hi Mike, On 06/26/2017 10:37 PM, Mike Snitzer wrote: <blockquote type="cite">On Mon, Jun 26 2017 at 9:47am -0400, Eric Ren <a class="moz-txt-link-rfc2396E" href="mailto:zren@suse.com"><zren@suse.com></a> wrote: [...snip...] <blockquote type="cite"> <blockquote type="cite">""" Revert "dm mirror: use all available legs on multiple failures" dm io: fix duplicate bio completion due to missing ref count </blockquote> I have a confusion about this "dm io..." fix. The fix itself is good. Without it, a mkfs.ext4 on a mirrored dev whose primary mirror dev has failed, will crash the kernel with the discard operation from mkfs.ext4. However, mkfs.ext4 can succeed on a healthy mirrored device. This is the thing I don't understand, because no matter the mirrored device is good or not, there's always a duplicate bio completion before having this this fix, thus write_callback() will be called twice, crashing will occur on the second write_callback(): </blockquote> No, there is only a duplicate bio completion if the error path is taken (e.g. underlying device doesn't support discard). </blockquote> Hmm, when "op == REQ_OP_DISCARD", please see comments in do_region(): """ static void do_region(int op, int op_flags, unsigned region, struct dm_io_region *where, struct dpages *dp, struct io *io) { ... if (op == REQ_OP_DISCARD) special_cmd_max_sectors = q->limits.max_discard_sectors; ... if ((op == REQ_OP_DISCARD || op == REQ_OP_WRITE_ZEROES || op == REQ_OP_WRITE_SAME) && special_cmd_max_sectors == 0) { atomic_inc(&io->count); ===> [1] dec_count(io, region, -EOPNOTSUPP); ===> [2] return; } """ [1] ref count fixed by patch "dm io: ..."; [2] we won't come here if "special_cmd_max_sectors != 0", which is true when both sides of the mirror is good. So only when a mirror device fails, "max_discard_sectors" on its queue is 0, thus this error path will be taken, right? </blockquote> Yes, I checked this as below: - print info """ <div id="magicdomid251" class="ace-line">@@ -310,8 +310,10 @@ static void do_region(int op, int op_flags, unsigned region,</div> <div id="magicdomid252" class="ace-line"> /*</div> <div id="magicdomid253" class="ace-line"> * Reject unsupported discard and write same requests.</div> <div id="magicdomid254" class="ace-line"> */</div> <div id="magicdomid255" class="ace-line">- if (op == REQ_OP_DISCARD)</div> <div id="magicdomid256" class="ace-line">+ if (op == REQ_OP_DISCARD) {</div> <div id="magicdomid257" class="ace-line"> special_cmd_max_sectors = q->limits.max_discard_sectors;</div> <div id="magicdomid258" class="ace-line">+ DMERR("do_region: op=DISCARD, q->limits.max_discard_sectors=%d\n", special_cmd_max_sectors);</div> <div id="magicdomid259" class="ace-line">+ }</div> """ - log message when mkfs on mirror device that primary leg fails: [ 169.672880] device-mapper: io: do_region: op=DISCARD, q->limits.max_discard_sectors=0 ===> for the failed leg [ 169.672885] device-mapper: io: do_region: op=DISCARD, q->limits.max_discard_sectors=8388607 ===> for the good leg Thanks, Eric <blockquote cite="mid:a2dcb2e0-cba2-6776-f041-605a66486ec5@suse.com" type="cite"> <blockquote type="cite"> <blockquote type="cite">""" static void write_callback(unsigned long error, void *context) { unsigned i; struct bio *bio = (struct bio *) context; struct mirror_set *ms; int should_wake = 0; unsigned long flags; ms = bio_get_m(bio)->ms; ====> NULL pointer at the duplicate completion bio_set_m(bio, NULL); """ If no this fix, I expected the DISCARD IO would always crash the kernel, but it's not true when the mirrored device is good. Hope someone happen to know the reason can give some hints ;-P </blockquote> If the mirror is healthy then only one completion is returned to dm-mirror (via write_callback). The problem was the error patch wasn't managing the reference count as needed. Whereas dm-io's normal discard IO path does. </blockquote> Yes, I can understand this. Thanks a lot, Eric <blockquote type="cite"> Mike -- dm-devel mailing list <a class="moz-txt-link-abbreviated" href="mailto:dm-devel@redhat.com">dm-devel@redhat.com</a> <a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/dm-devel">https://www.redhat.com/mailman/listinfo/dm-devel</a> </blockquote> -- dm-devel mailing list <a class="moz-txt-link-abbreviated" href="mailto:dm-devel@redhat.com">dm-devel@redhat.com</a> <a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/dm-devel">https://www.redhat.com/mailman/listinfo/dm-devel</a> </blockquote> </body> </html>