Re: [PATCH 1/2] staging: zram: minimize `slot_free_lock' usage (v2)

[Dan Carpenter <dan.carpenter@oracle.com>]

On Mon, Sep 09, 2013 at 03:49:42PM +0300, Sergey Senozhatsky wrote:
> > > Calling handle_pending_slot_free() for every RW operation may
> > > cause unneccessary slot_free_lock locking, because most likely
> > > process will see NULL slot_free_rq. handle_pending_slot_free()
> > > only when current detects that slot_free_rq is not NULL.
> > > 
> > > v2: protect handle_pending_slot_free() with zram rw_lock.
> > > 
> > 
> > zram->slot_free_lock protects zram->slot_free_rq but shouldn't the zram
> > rw_lock be wrapped around the whole operation like the original code
> > does?  I don't know the zram code, but the original looks like it makes
> > sense but in this one it looks like the locks are duplicative.
> > 
> > Is the down_read() in the original code be changed to down_write()?
> >
> 
> I'm not touching locking around existing READ/WRITE commands.
> 

Your patch does change the locking because now instead of taking the
zram lock once it takes it and then drops it and then retakes it.  This
looks potentially racy to me but I don't know the code so I will defer
to any zram maintainer.

1) You haven't given us any performance numbers so it's not clear if the
   locking is even a problem.

2) The v2 patch introduces an obvious deadlock in zram_slot_free()
   because now we take the rw_lock twice.  Fix your testing to catch
   this kind of bug next time.

3) Explain why it is safe to test zram->slot_free_rq when we are not
   holding the lock.  I think it is unsafe.  I don't want to even think
   about it without the numbers.

regards,
dan carpenter