Re: [PATCH v2] zfcp: fix reaction on bit error theshold notification with adapter close

From: Greg KH <gregkh@linuxfoundation.org>
To: Steffen Maier <maier@linux.ibm.com>
Cc: "James E . J . Bottomley" <jejb@linux.ibm.com>,
	"Martin K . Petersen" <martin.petersen@oracle.com>,
	linux-scsi@vger.kernel.org, linux-s390@vger.kernel.org,
	Benjamin Block <bblock@linux.ibm.com>,
	Heiko Carstens <heiko.carstens@de.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH v2] zfcp: fix reaction on bit error theshold notification with adapter close
Date: Tue, 1 Oct 2019 17:42:08 +0200	[thread overview]
Message-ID: <20191001154208.GB3523275@kroah.com> (raw)
In-Reply-To: <71b8fc68-23a8-a591-1018-f290d6e3312a@linux.ibm.com>

On Tue, Oct 01, 2019 at 05:07:50PM +0200, Steffen Maier wrote:
> On 10/1/19 4:14 PM, Greg KH wrote:
> > On Tue, Oct 01, 2019 at 12:49:49PM +0200, Steffen Maier wrote:
> > > On excessive bit errors for the FCP channel ingress fibre path, the channel
> > > notifies us. Previously, we only emitted a kernel message and a trace record.
> > > Since performance can become suboptimal with I/O timeouts due to
> > > bit errors, we now stop using an FCP device by default on channel
> > > notification so multipath on top can timely failover to other paths.
> > > A new module parameter zfcp.ber_stop can be used to get zfcp old behavior.
> > 
> > Ugh, module parameters?  This isn't the 1990's anymore :(
> > 
> > Why not just make this a dynamic sysfs variable, that way you properly
> > can set this on whatever device you want, not just "all or nothing"?
> 
> Since we can see many more (virtual) FCP devices than we want to actually
> use, we defer probing. It means, we only start allocating structures and
> sysfs entries on setting an FCP "online" for the first time. Setting online
> works through another sysfs attribute owned by our ccw bus code component
> called "cio". IIRC, setting online does not emit a uevent. On setting
> online, the (add) uevent of hot-/coldplug of an FCP device had already
> happened, so we could not easily have end users craft udev rules to
> automatically/persistently configure a new sysfs attribute (which is
> FCP-device-specific and appears late) to disable the new code behavior.
> 
> Not sure if that could ever become a problem for end users: Even if we were
> to write into a new sysfs attribute, the attribute only appears during
> setting online so this might race with starting to actually use the FCP
> device with the new default behavior and could potentially disable I/O paths
> before the sysfs attribute write could become effective to disable the new
> behavor.

Ok, then why make this a module option that you will have to support for
the next 20+ years anyway if you feel this fix is the correct way that
it should be done instead?

module options are tough to manage and support, only add them as a very
last thing, when all other options have been ruled out.

thanks,

greg k-h