On 5/2/20 5:11 AM, Ming Lei wrote:
On Fri, May 01, 2020 at 07:45:05PM +0200, Christoph Hellwig wrote:
On Fri, May 01, 2020 at 11:01:29PM +0800, Ming Lei wrote:
We cannot increase MAX_QUEUE arbitrarily as this is a compile time variable,
which seems to relate to a hardware setting.
But I can see to update the reserved command functionality for allowing to
fetch commands from the normal I/O tag pool; in the case of LUN reset it
shouldn't make much of a difference as the all I/O is quiesced anyway.
It isn't related with reset.
This patch reduces active IO queue depth by 1 anytime no matter there is reset
or not, and this way may cause performance regression.
But isn't it the right thing to do? How else do we guarantee that
there always is a tag available for the LU reset?
If that is case, some of these patches should be bug-fix, but nothing
about this kind of comment is provided. If it is true, please update
the commit log and explain the current issue in detail, such as,
what is the side-effect of 'overwriting the original command'?
And we might need to backport it to stable tree because storage error
recovery is very key function.
> Even though it is true, still not sure if this patch is the correct
way to fix the issue cause IO performance drop might be caused.
You can't have it both ways.
The underlying problem is this:
The csiostor driver (and several others, too) require a valid hardware
tag to send a LU reset command. Currently it tries to allocate a tag
from the pool of free hardware tags.
However, experience shows that the majority of cases (in my personal
experience _all_ of the cases) where we ever entered the error handler
are due to command timeouts. If now all commands timed out, the tag
space is full and we cannot get a free tag to send the LU reset command.
Hence LU reset will currently fail in this case.
With the patchset we will always ensure to have at least one free tag
such that we can send the LU reset command. But, as correctly noted,
it will reduce the available tagspace and possibly reduce the performance.
But you really can have it both ways. Either you go for max performance
and have the risk of starving the error handler, or you go for
reliability and accept a (slightly) lower performance.
And, btw, I'm not sure if one could even measure the performance impact.
csiostor has 2048 tags per HBA with a 10G FCoE link. So it would require
a latency of less than 8us with 4k I/O to saturate the HBA; for
everything slower we wouldn't be seeing anything.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
