Re: [PATCH v2 0/4] zram: fix few sysfs races

From: Luis Chamberlain <mcgrof@kernel.org>
To: Minchan Kim <minchan@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Hannes Reinecke <hare@suse.de>,
	Douglas Gilbert <dgilbert@interlog.com>
Cc: ngupta@vflare.org, sergey.senozhatsky.work@gmail.com,
	axboe@kernel.dk, mbenes@suse.com, jpoimboe@redhat.com,
	tglx@linutronix.de, keescook@chromium.org, jikos@kernel.org,
	rostedt@goodmis.org, peterz@infradead.org,
	linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 0/4] zram: fix few sysfs races
Date: Wed, 19 May 2021 20:20:23 +0000	[thread overview]
Message-ID: <20210519202023.GU4332@42.do-not-panic.com> (raw)
In-Reply-To: <YKVwZVcbZBNXUpKm@google.com>

Greg,

your feedback would be appreciated here.

On Wed, May 19, 2021 at 01:09:09PM -0700, Minchan Kim wrote:
> On Fri, Apr 23, 2021 at 01:11:04AM +0000, Luis Chamberlain wrote:
> > This 2nd series documents the fixes better and includes a bdgrab() fix
> > for the issue noted by Minchan. A general fix has been proposed for two
> > of these issues however they are not yet deemed required upstream and so
> > we just open code individual solutions on the driver.
> > 
> > Luis Chamberlain (4):
> >   zram: fix crashes due to use of cpu hotplug multistate
> >   zram: avoid disksize setting when device is being claimed
> >   zram: fix deadlock with sysfs attribute usage and driver removal
> >   zram: fix possible races between sysfs use and bdev access
> > 
> >  drivers/block/zram/zram_drv.c | 473 +++++++++++++++++++++++++++++-----
> >  1 file changed, 414 insertions(+), 59 deletions(-)
> 
> Hi Luis,
> 
> First of all, I am sorry too late review. Now I see [3/4] and [4/4] would
> be not only zram issue since you shed a light in the descriptions.
> Yeah, that would be helpful if it could be deal with under general
> layer but looks like arguable or would take some times at least, IIUC.
> 
> On the case, yeah, we could fix it for zram first until the issue will
> bring up further. Anyway, I'd like to see some wrapper rather than annotating
> for every sysfs files for maintainance point of view.
> At least, could you introduce one more patch "introduce zram sysfs wrapper"
> on top of this series to centralize the work?
> 
> Thanks for your works!

Since I did the work for a general fix as an alternative proof of
concept to the ugliness reflected on those two last patches, I'd like
instead for Greg to re-consider merging a general fix.

Greg, can you comment on technical levels why a general core fix is not
desirable upstream for those two issues?

Based on feedback from folks, it does not seem the argument that we
don't support rmmod holds water.  Specially given we are now even
discussing using live patching to suport error injection as a strategy
to not incur more code / boiler plate code even on the block layer [0].
The live patching approach would mean for testing purposes we'd
temporarily use error injection via live patching and then remove the
module after done testing.

[0] http://lkml.kernel.org/r/989175a7-5533-02ef-c096-b24b2769c9cf@suse.de

  Luis