[dm-devel] Unnecessary scanning of all SCSI devices in mpath_persistent_reserve_in/out command, sent to a specific mpath device,

[Christophe Varoqui]

Any comment on this mpath_persist optimization suggested by Satyajit ? I'd
be inclined to merge it.

Best regards,
Christophe Varoqui

diff -uNr multipath-tools/libmpathpersist/mpath_persist.c
multipath-tools-2/libmpathpersist/mpath_persist.c
--- multipath-tools/libmpathpersist/mpath_persist.c	2015-08-27
22:58:20.938838600 -0700
+++ multipath-tools-2/libmpathpersist/mpath_persist.c	2015-08-27
22:59:42.763466711 -0700
@@ -182,7 +182,7 @@
 		goto out;
 	}

-	if (path_discovery(pathvec, conf, DI_SYSFS | DI_CHECKER)) {
+	if (path_discovery(pathvec, conf, DI_SYSFS)) {
 		ret = MPATH_PR_DMMP_ERROR;
 		goto out1;
 	}
@@ -272,7 +272,7 @@
                 goto out;
         }

-	if (path_discovery(pathvec, conf, DI_SYSFS | DI_CHECKER)) {
+	if (path_discovery(pathvec, conf, DI_SYSFS)) {
 		ret = MPATH_PR_DMMP_ERROR;
 		goto out1;
 	}


On Mon, Aug 31, 2015 at 10:54 AM, Satyajit Deshmukh <
satyajit.s.deshmukh at gmail.com> wrote:

> Quick update on this issue-
> We spoke to Hannes Reinecke and understood that we were using a
> sub-optimal checker aka 'directio' for testing the paths.
> As recommended to us by Hannes, we tried the 'tur' (Test Unit Ready)
> instead of the 'directio'.
> Result: We saw huge improvements in the times for the PROUT/PRIN commands.
> This was the change needed:
> libmultipath/checkers.h
> - #define DEFAULT_CHECKER DIRECTIO
> + #define DEFAULT_CHECKER TUR
>
> However, I still believe that the path_discovery() API should not have the
> DI_CHECKER bit in the flags passed to it.
> By removing this bit, we saw even better times (about 3x faster) for the
> PROUT/PRIN commands.
> Thus, if there are no side-effects/repercussions of removing this bit from
> the place I mentioned, we could remove the bit IMO.
>
> Thanks,
> Satyajit
>
> On Fri, Aug 28, 2015 at 12:49 AM, Satyajit Deshmukh <
> satyajit.s.deshmukh at gmail.com> wrote:
>
>> Hello,
>>
>> This is related to an issue raised by Richard Sharpe recently.
>> https://www.redhat.com/archives/dm-devel/2015-August/msg00154.html
>>
>> The RPM that was used-
>>
>> http://vault.centos.org/6.6/updates/Source/SPackages/device-mapper-multipath-0.4.9-80.el6_6.3.src.rpm
>>
>> *Problem:*
>> From reading the source code, I understand that 2 APIs defined in
>> libmpathpersist/mpath_persist.c do a system-wide scan of all SCSI devices
>> by sending asynchronous IO (io_submit calls) to test if SCSI device path is
>> good:
>>
>>
>>
>> *extern int mpath_persistent_reserve_in (int fd, int rq_servact, struct
>> prin_resp *resp, int noisy, int verbose);extern int
>> mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,
>> unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy,
>> int verbose);*
>>
>> So, to get a persistent reservation on a single /dev/mapper/mpathx device
>> on a system having 400 SCSI devices, current code does 400 reads of 4K
>> each.(512 bytes of 8 sectors), leading to huge delays.
>>
>> *Root cause of the system wide device scan:*
>> The system-wide scan is triggered by setting the DI_CHECKER bit in the
>> flags argument to a path_discovery() API:
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> * if (path_discovery(pathvec, conf, DI_SYSFS | DI_CHECKER)) {    ret =
>> MPATH_PR_DMMP_ERROR;    goto out1;  }  /* get info of all paths from the dm
>> device */  if (get_mpvec (curmp, pathvec, alias)){    condlog(0, "%s:
>> failed to get device info.", alias);    ret = MPATH_PR_DMMP_ERROR;    goto
>> out1;  }  *
>>
>> I believe the DI_CHECKER bit in path_discovery() API is not needed.
>> This is because the path status checking is already done on the specific
>> mpath device in the get_mpvec API.
>>
>> The get_mpvec() gets the required struct multipath *mpp, and then invokes
>> pathinfo(pp, conf->hwtable, DI_CHECKER) for each path in the multipath
>> dev.
>> And this checks state of each path for the multipath device.
>>
>> * if (mask & DI_CHECKER) {    pp->chkrstate = pp->state = get_state(pp,
>> 0);*
>>
>> *Possible solution:*
>> I have a patch ready that simply removes the DI_CHECKER bit from the
>> path_discovery() calls inside mpath_persistent_reserve_in() and
>> mpath_persistent_reserve_out(). Attached the patch.
>> I have tested out the following PRIN and PROUT commands and don't see any
>> errors.
>> mpathpersist -i -k /dev/mapper/mpathig
>> mpathpersist -i -r /dev/mapper/mpathig
>> mpathpersist --out --register --param-rk=0A041D3d /dev/mapper/mpathig
>> mpathpersist --out --register --param-sark=0A041D3d /dev/mapper/mpathig
>> mpathpersist --out --reserve --param-rk=0A041D3D --prout-type=5
>> --verbose=3 /dev/mapper/mpathig
>>
>> Request someone to review the patch, suggest changes and guide me in
>> fixing this issue.
>>
>> Thanks,
>> Satyajit
>>
>
>
> --
> dm-devel mailing list
> dm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20151006/c30b9e59/attachment.htm>