Re: [PATCH v2 2/4] dax/bus.c: fix locking for unregister_dax_dev / unregister_dax_mapping paths

[Dan Williams]

Verma, Vishal L wrote:
> > > @@ -560,15 +551,12 @@ static ssize_t delete_store(struct device *dev, struct device_attribute *attr,
> > >  	if (!victim)
> > >  		return -ENXIO;
> > >  
> > > -	rc = down_write_killable(&dax_region_rwsem);
> > > -	if (rc)
> > > -		return rc;
> > > -	rc = down_write_killable(&dax_dev_rwsem);
> > > -	if (rc) {
> > > -		up_write(&dax_region_rwsem);
> > > -		return rc;
> > > -	}
> > > +	device_lock(dev);
> > > +	device_lock(victim);
> > >  	dev_dax = to_dev_dax(victim);
> > > +	rc = down_write_killable(&dax_dev_rwsem);
> > 
> > This begs the question, why down_write_killable(), but not
> > device_lock_interruptible()?
> 
> Do you mean change the device_lock()s to device_lock_interruptible() in
> addition to the taking the rwsem (i.e. not instead of the rwsem..)?

I mean convert the rwsem to drop _killable.

> I guess I just restored what was there previously - but the
> interruptible variant makes sense, I can make that change.

So the original code did device_lock(), then the rework added killable
rwsem (deleted device_lock()), and now the fixes add device_lock() back.
So now that there is a mix of killable/interruptible lock usage all the
locks should agree.

Since there really is no risk of these operations being long running
there is no driving need to make them killable/interruptible, so go with
the simple option.