Re: Unnecessary scanning of all SCSI devices in mpath_persistent_reserve_in/out command, sent to a specific mpath device,

[Christophe Varoqui <christophe.varoqui@xxxxxxxxxxx>]

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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel

--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel