On (09/09/13 18:10), Jerome Marchand wrote: > On 09/09/2013 03:46 PM, Jerome Marchand wrote: > > On 09/09/2013 03:21 PM, Dan Carpenter wrote: > >> 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. > > > > You're right. Nothing prevents zram_slot_free_notify() to repopulate the > > free slot queue while we drop the lock. > > > > Actually, the original code is already racy. handle_pending_slot_free() > > modifies zram->table while holding only a read lock. It needs to hold a > > write lock to do that. Using down_write for all requests would obviously > > fix that, but at the cost of read performance. > > Now I think we can drop the call to handle_pending_slot_free() in > zram_bvec_rw() altogether. As long as the write lock is held when > handle_pending_slot_free() is called, there is no race. It's no different > from any write request and the current code handles R/W concurrency > already. Yes, I think that can work. To summarize, there should be 3 patches: 1) handle_pending_slot_free() in zram_bvec_rw() (as suggested by Jerome Marchand) 2) handle_pending_slot_free() race with reset (found by Dan Carpenter) 3) drop init_done and use init_done() I'll prepare a patches later today. -ss > Jerome > > > > >> > >> 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 > >> > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@xxxxxxxxxxxxxxx > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > > > _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
