2010-12-30 09:08:16

by Nitin Gupta

[permalink] [raw]
Subject: [PATCH][zram] Do not check for init flag before starting I/O

zram module is unusable without this fix.

Checking for init_done flag in zram_make_request() results in a
*no-op* for any I/O operation since we simply always return
success. This flag is actually set when the first write occurs
on a zram disk which triggers its initialization.

Bug report: https://bugzilla.kernel.org/show_bug.cgi?id=25722

Reported-by: Dennis Jansen <[email protected]>
Signed-off-by: Nitin Gupta <[email protected]>
---
drivers/staging/zram/zram_drv.c | 6 ------
1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 8c3c057..d0e9e02 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -435,12 +435,6 @@ static int zram_make_request(struct request_queue *queue, struct bio *bio)
int ret = 0;
struct zram *zram = queue->queuedata;

- if (unlikely(!zram->init_done)) {
- set_bit(BIO_UPTODATE, &bio->bi_flags);
- bio_endio(bio, 0);
- return 0;
- }
-
if (!valid_io_request(zram, bio)) {
zram_stat64_inc(zram, &zram->stats.invalid_io);
bio_io_error(bio);
--
1.7.3.4


2010-12-30 17:17:54

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH][zram] Do not check for init flag before starting I/O

On Thu, Dec 30, 2010 at 1:07 AM, Nitin Gupta <[email protected]> wrote:
> zram module is unusable without this fix.

.. and apparently it oopses without it.

See commit 7e24cce38a99f3 which added the code that you now remove.
You were cc'd on it, I don't think we got any reply to it.

So no. I'm not taking this without way more explanations of why the
original problem isn't a problem any more.

Linus

2010-12-30 19:02:48

by Nitin Gupta

[permalink] [raw]
Subject: Re: [PATCH][zram] Do not check for init flag before starting I/O

On 12/30/2010 10:47 PM, Linus Torvalds wrote:
> On Thu, Dec 30, 2010 at 1:07 AM, Nitin Gupta<[email protected]> wrote:
>> zram module is unusable without this fix.
>
> .. and apparently it oopses without it.
>
> See commit 7e24cce38a99f3 which added the code that you now remove.
> You were cc'd on it, I don't think we got any reply to it.
>

This commit shows oops in zram_inc_stat() which does not exist in
staging tree version of zram. Its actually a problem with zram version
as present in project's own repository where we allocate struct
zram_stats_cpu upon device initialization. OTOH, In mainline/staging
version of zram, we allocate struct stats upfront, so this oops cannot
happen in mainline version.

So, this commit is not even applicable for mainline/staging tree. My
bad, I didn't get a chance to review that patch.

> So no. I'm not taking this without way more explanations of why the
> original problem isn't a problem any more.
>

I will now end development on local project repository and sync it with
the mainline version, so all future development happens on
mainline/staging only, avoiding such confusions in future.


Nitin

2010-12-30 20:09:25

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH][zram] Do not check for init flag before starting I/O

On Thu, Dec 30, 2010 at 11:02 AM, Nitin Gupta <[email protected]> wrote:
>
> This commit shows oops in zram_inc_stat() which does not exist in staging
> tree version of zram. Its actually a problem with zram version as present in
> project's own repository where we allocate struct zram_stats_cpu upon device
> initialization. OTOH, In mainline/staging version of zram, we allocate
> struct stats upfront, so this oops cannot happen in mainline version.
>
> So, this commit is not even applicable for mainline/staging tree. ?My bad, I
> didn't get a chance to review that patch.

Ok. So it really is a revert, and the commit message for the revert
should have explained that.

I changed the commit message appropriately, and committed it as the
attached commit.

Linus


Attachments:
patch.diff (6.34 kB)