On 2016/2/4 11:24, Mike Snitzer wrote: > On Wed, Feb 03 2016 at 9:08pm -0500, > jiangyiwen <jiangyiwen@xxxxxxxxxx> wrote: > >> When two processes submit WRTIE SAME bio simultaneously and >> first IO return failed because of INVALID FIELD IN CDB, and >> then second IO can enter into an infinite loop. >> The problem can be described as follows: >> >> process 1 process 2 >> submit_bio(REQ_WRITE_SAME) and >> wait io completion >> begin submit_bio(REQ_WRITE_SAME) >> -> blk_queue_bio() >> -> dm_request_fn() >> -> map_request() >> -> scsi_request_fn() >> -> blk_peek_request() >> -> scsi_prep_fn() >> In the moment, IO return and >> sense_key with illegal_request, >> sense_code with 0x24(INVALID FIELD IN CDB). >> In this way it will set no_write_same = 1 >> in sd_done() and disable write same >> In sd_setup_write_same_cmnd() >> when find (no_write_same == 1) >> will return BLKPREP_KILL, >> and then in blk_peek_request() >> set error to EIO. >> However, in multipath_end_io() >> when find error is EIO it will >> fail path and retry even if >> device doesn't support WRITE SAME. >> >> In this situation, when process of multipathd reinstate >> path by using test_unit_ready periodically, it will cause >> second WRITE SAME IO enters into an infinite loop and >> loop never terminates. >> >> In do_end_io(), when finding device doesn't support WRITE SAME and >> request is WRITE SAME, return EOPNOTSUPP to applications. >> >> Signed-off-by: Yiwen Jiang <jiangyiwen@xxxxxxxxxx> >> Reviewed-by: Joseph Qi <joseph.qi@xxxxxxxxxx> >> Reviewed-by: xuejiufei <xuejiufei@xxxxxxxxxx> >> --- >> drivers/md/dm-mpath.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c >> index cfa29f5..ad1602a 100644 >> --- a/drivers/md/dm-mpath.c >> +++ b/drivers/md/dm-mpath.c >> @@ -1269,6 +1269,11 @@ static int do_end_io(struct multipath *m, struct request *clone, >> if (noretry_error(error)) >> return error; >> >> + /* do not retry in case of WRITE SAME not supporting */ >> + if ((clone->cmd_flags & REQ_WRITE_SAME) && >> + !clone->q->limits.max_write_same_sectors) >> + return -EOPNOTSUPP; >> + >> if (mpio->pgpath) >> fail_path(mpio->pgpath); >> > > Did you test this patch? Looks like it isn't going to make a > difference. 'error' will be -EREMOTEIO, which will be caught by > noretry_error(). So we'll never go on to run your new code. > > Please see commit 7eee4ae2db ("dm: disable WRITE SAME if it fails"). > > I think DM already handles this case properly. The only thing is it'll > return -EREMOTEIO (rather than -EOPNOTSUPP) further up the stack. > > . > Hi Mike, Yes, I have already test in version linux-3.8, and I also have already carefully checked latest kernel code, and find that it also exists this problem. I know about this commit 7eee4ae2db ("dm: disable WRITE SAME if it fails"), it only can solve first IO situation which I mentioned above, in other words, when WRITE SAME IO truly send to device, it actually return -EREMOTEIO if device doesn't support WRITE SAME. But in above situation which issues two WRITE SAME IO simultaneously, second IO will not truly send to device instead of returning BLKPREP_KILL in sd_setup_write_same_cmnd() because find (no_write_same == 1) which no_write_same is set when first IO returned, and then in blk_peek_request() will return EIO to MD/DM which caused the problem above mentioned. I have described detailed process of problem, you will find actually it is a problem when reviewed carefully. Thanks, Yiwen Jiang -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel