Hello,
On (09/16/13 09:02), Minchan Kim wrote:
> Hello Sergey,
>
> Sorry for really slow response. I was really busy by internal works
> and Thanks for pointing the BUG, Dan, Jerome and Sergey.
> I read your threads roughly so I may miss something. If so, sorry
> for that. Anyway I will put my opinion.
>
> On Wed, Sep 11, 2013 at 02:12:50AM +0300, Sergey Senozhatsky wrote:
> > Dan Carpenter noted that handle_pending_slot_free() is racy with
> > zram_reset_device(). Take write init_lock in zram_slot_free(), thus
>
> Right but "init_lock" is what I really want to remove.
> Yes. It's just read-side lock so most of time it doesn't hurt us but it
> makes code very complicated and deadlock prone so I'd like to replace
> it with RCU. Yeah, It's off topic but just let me put my opinion in
> future direction.
>
> Abought the bug, how about moving flush_work below down_write(init_lock)?
> zram_make_request is already closed by init_lock and we have a rule about
> lock ordering as following so I don't see any problem.
>
> init_lock
> zram->lock
>
> > preventing any concurrent zram_slot_free(), zram_bvec_rw() or
> > zram_reset_device(). This also allows to safely check zram->init_done
> > in handle_pending_slot_free().
> >
> > Initial intention was to minimze number of handle_pending_slot_free()
> > call from zram_bvec_rw(), which were slowing down READ requests due to
> > slot_free_lock spin lock. Jerome Marchand suggested to remove
> > handle_pending_slot_free() from zram_bvec_rw().
> >
> > Link: https://lkml.org/lkml/2013/9/9/172
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhat...@gmail.com>
> >
> > ---
> >
> > drivers/staging/zram/zram_drv.c | 13 +++++--------
> > 1 file changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/staging/zram/zram_drv.c
> > b/drivers/staging/zram/zram_drv.c
> > index 91d94b5..7a2d4de 100644
> > --- a/drivers/staging/zram/zram_drv.c
> > +++ b/drivers/staging/zram/zram_drv.c
> > @@ -521,7 +521,8 @@ static void handle_pending_slot_free(struct zram *zram)
> > while (zram->slot_free_rq) {
> > free_rq = zram->slot_free_rq;
> > zram->slot_free_rq = free_rq->next;
> > - zram_free_page(zram, free_rq->index);
> > + if (zram->init_done)
> > + zram_free_page(zram, free_rq->index);
> > kfree(free_rq);
> > }
> > spin_unlock(&zram->slot_free_lock);
> > @@ -534,16 +535,13 @@ static int zram_bvec_rw(struct zram *zram, struct
> > bio_vec *bvec, u32 index,
> >
> > if (rw == READ) {
> > down_read(&zram->lock);
> > - handle_pending_slot_free(zram);
>
> Read side is okay but actually I have a nitpick.
> If someone poll a block in zram-swap device, he would see a block
> has zero value suddenly although there was no I/O.(I don't want to argue
> it's sane user or not, anyway) it never happens on real block device and
> it never happens on zram-block device. Only it can happen zram-swap device.
> And such behavior was there since we introduced swap_slot_free_notify.
> (off-topic: I'd like to remove it because it makes tight coupling between
> zram and swap and obviously, it was layering violation function)
> so now, I don't have strong objection.
>
> The idea is to remove swap_slot_free_notify is to use frontswap when
> user want to use zram as swap so zram can be notified when the block
> lose the owner but still we should solve the mutex problem in notify
> handler.
>
>
> > ret = zram_bvec_read(zram, bvec, index, offset, bio);
> > up_read(&zram->lock);
> > } else {
> > down_write(&zram->lock);
> > - handle_pending_slot_free(zram);
>
> Why did you remove this in write-side?
> We can't expect when the work will trigger. It means the work could remove
> valid block under the us.
>
not sure I understand how.
zram_slot_free() takes down_write(&zram->init_lock) and zram_make_request()
takes
down_read(&zram->init_lock), thus zram_slot_free() can not concurrently work
with
any RW requests. RW requests are under read() lock and zram_slot_free() is under
write() lock.
> > ret = zram_bvec_write(zram, bvec, index, offset);
> > up_write(&zram->lock);
> > }
> > -
> > return ret;
> > }
> >
> > @@ -750,12 +748,11 @@ error:
> >
> > static void zram_slot_free(struct work_struct *work)
> > {
> > - struct zram *zram;
> > + struct zram *zram = container_of(work, struct zram, free_work);
> >
> > - zram = container_of(work, struct zram, free_work);
> > - down_write(&zram->lock);
> > + down_write(&zram->init_lock);
>
> I don't like this.
> Primary problem is we should handle it as atomic so that we should use
> spinlock instead of mutex. Yeah, /me kicks his ass. From the beginning,
> I should solve this problem as that way.
>
> The simple solution popped from my mind is that
>
>
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 91d94b5..b23bf0e 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -534,11 +534,14 @@ static int zram_bvec_rw(struct zram *zram, struct
> bio_vec *bvec, u32 index,
>
> if (rw == READ) {
> down_read(&zram->lock);
> - handle_pending_slot_free(zram);
> ret = zram_bvec_read(zram, bvec, index, offset, bio);
> up_read(&zram->lock);
> } else {
> down_write(&zram->lock);
> + /*
> + * We should free pending slot. Otherwise it would
> + * free valid blocks under the us.
> + */
> handle_pending_slot_free(zram);
> ret = zram_bvec_write(zram, bvec, index, offset);
> up_write(&zram->lock);
> @@ -552,7 +555,6 @@ static void zram_reset_device(struct zram *zram, bool
> reset_capacity)
> size_t index;
> struct zram_meta *meta;
>
> - flush_work(&zram->free_work);
>
> down_write(&zram->init_lock);
> if (!zram->init_done) {
> @@ -560,6 +562,7 @@ static void zram_reset_device(struct zram *zram, bool
> reset_capacity)
> return;
> }
>
> + flush_work(&zram->free_work);
> meta = zram->meta;
> zram->init_done = 0;
this one looks ok to me.
-ss
> But more ideal way I am thinking now is
>
> 1) replace init_lock with RCU lock
> 2) introduce new meta atmoic lock instead of zram->mutex, which is very
> coarse-grained.
> 3) use atmoic lock in notify handler.
>
> --
> Kind regards,
> Minchan Kim
>
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
