Re: [PATCH 1/2] zram: fix crashes due to use of cpu hotplug multistate

[Minchan Kim]

On Tue, Apr 06, 2021 at 06:23:46PM -0700, Minchan Kim wrote:
> On Tue, Apr 06, 2021 at 12:29:09AM +0000, Luis Chamberlain wrote:
> > On Mon, Apr 05, 2021 at 12:58:05PM -0700, Minchan Kim wrote:
> > > On Mon, Apr 05, 2021 at 07:00:23PM +0000, Luis Chamberlain wrote:
> > > > On Mon, Apr 05, 2021 at 10:07:24AM -0700, Minchan Kim wrote:
> > > > > On Thu, Apr 01, 2021 at 11:59:25PM +0000, Luis Chamberlain wrote:
> > > > > > And come to think of it the last patch I had sent with a new
> > > > > > DECLARE_RWSEM(zram_unload) also has this same issue making most
> > > > > > sysfs attributes rather fragile.
> > > > > 
> > > > > Thanks for looking the way. I agree the single zram_index_rwlock is
> > > > > not the right approach to fix it. However, I still hope we find more
> > > > > generic solution to fix them at once since I see it's zram instance
> > > > > racing problem.
> > > > 
> > > > They are 3 separate different problems. Related, but different.
> > > 
> > > What are 3 different problems? I am asking since I remember only two:
> > > one for CPU multistate and the other one for sysfs during rmmod.
> > 
> > The third one is the race to use sysfs attributes and those routines
> > then derefernece th egendisk private_data.
> 
> First of all, thanks for keeping discussion, Luis.
> 
> That was the one I thought race between sysfs and during rmmod.
> 
> > 
> > > > If the idea then is to busy out rmmod if a sysfs attribute is being
> > > > read, that could then mean rmmod can sometimes never complete. Hogging
> > > > up / busying out sysfs attributes means the module cannto be removed.
> > > 
> > > It's true but is it a big problem? There are many cases that system
> > > just return error if it's busy and rely on the admin. IMHO, rmmod should
> > > be part of them.
> > 
> > It depends on existing userspace scripts which are used to test and
> > expectations set. Consider existing tests, you would know better, and
> > since you are the maintainer you decide.
> > 
> > I at least know for many other types of device drivers an rmmod is
> > a sledge hammer.
> > 
> > You decide. I just thought it would be good to highlight the effect now
> > rather than us considering it later.
> 
> To me, the rmmod faillure is not a big problem for zram since it's
> common cases in the system with -EBUSY(Having said, I agree that's the
> best if we could avoid the fail-and-retrial. IOW, -EBUSY should be
> last resort unless we have nicer way.)
> 
> > 
> > > > Which is why the *try_module_get()* I think is much more suitable, as
> > > > it will always fails if we're already going down.
> > > 
> > > How does the try_module_get solved the problem?
> > 
> > The try stuff only resolves the deadlock. The bget() / bdput() resolves
> > the race to access to the gendisk private_data.
> 
> That's the one I missed in this discussion. Now I am reading your [2/2]
> in original patch. I thought it was just zram instance was destroyed
> by sysfs race problem so you had seen the deadlock. I might miss the
> point here, too. 
> 
> Hmm, we are discussing several problems all at once. I feel it's time
> to jump v2 with your way in this point. You said three different
> problems. As I asked, please write it down with more detail with
> code sequence as we discussed other thread. If you mean a deadlock,
> please write what specific locks was deadlock with it.
> It would make discussion much easier. Let's discuss the issue
> one by one in each thread.

To clarify what I understood form the discussion until now:

1. zram shouldn't allow creating more zram instance during
rmmod(It causes CPU multistate splat)

2. the private data of gendisk shouldn't destroyed while zram
sysfs knob is working(it makes system goes crash)

Thank you.