[PATCH] zram: correct offset usage in zram_bio_discard

[PATCH] zram: correct offset usage in zram_bio_discard

[Weijie Yang]

we want to skip the logical block which is partially covered by
the discard bio, so check the remaining size and subtract it if
there is a need to goto the next logical block.

This patch corrects the offset usage in zram_bio_discard.

Signed-off-by: Weijie Yang <weijie.yang@samsung.com>
---
 drivers/block/zram/zram_drv.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 9849b52..48eccb3 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -572,10 +572,10 @@ static void zram_bio_discard(struct zram *zram, u32 index,
 	 * skipping this logical block is appropriate here.
 	 */
 	if (offset) {
-		if (n < offset)
+		if (n <= (PAGE_SIZE - offset))
 			return;
 
-		n -= offset;
+		n -= (PAGE_SIZE - offset);
 		index++;
 	}
 
-- 
1.7.10.4

Re: [PATCH] zram: correct offset usage in zram_bio_discard

[Sergey Senozhatsky]

On (04/22/14 11:14), Weijie Yang wrote:
> 
> we want to skip the logical block which is partially covered by
> the discard bio, so check the remaining size and subtract it if
> there is a need to goto the next logical block.
> 

looks sane.

	-ss

> This patch corrects the offset usage in zram_bio_discard.
> 
> Signed-off-by: Weijie Yang <weijie.yang@samsung.com>
> ---
>  drivers/block/zram/zram_drv.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 9849b52..48eccb3 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -572,10 +572,10 @@ static void zram_bio_discard(struct zram *zram, u32 index,
>  	 * skipping this logical block is appropriate here.
>  	 */
>  	if (offset) {
> -		if (n < offset)
> +		if (n <= (PAGE_SIZE - offset))
>  			return;
>  
> -		n -= offset;
> +		n -= (PAGE_SIZE - offset);
>  		index++;
>  	}
>  
> -- 
> 1.7.10.4
> 
> 

Re: [PATCH] zram: correct offset usage in zram_bio_discard

[Andrew Morton]

On Tue, 22 Apr 2014 11:14:02 +0800 Weijie Yang <weijie.yang@samsung.com> wrote:

> we want to skip the logical block which is partially covered by
> the discard bio, so check the remaining size and subtract it if
> there is a need to goto the next logical block.
> 
> This patch corrects the offset usage in zram_bio_discard.
> 

What were the end-user visible effects of the bug?

Please always include this information when fixing something so that
others can work out which kernel(s) need patching.

Re: [PATCH] zram: correct offset usage in zram_bio_discard

[Weijie Yang]

On Wed, Apr 23, 2014 at 3:55 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 22 Apr 2014 11:14:02 +0800 Weijie Yang <weijie.yang@samsung.com> wrote:
>
>> we want to skip the logical block which is partially covered by
>> the discard bio, so check the remaining size and subtract it if
>> there is a need to goto the next logical block.
>>
>> This patch corrects the offset usage in zram_bio_discard.
>>
>
> What were the end-user visible effects of the bug?
>
> Please always include this information when fixing something so that
> others can work out which kernel(s) need patching.
>

Thanks for your advise, I will resend this patch and add the end-user
visible effect information.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] zram: correct offset usage in zram_bio_discard

[Joonsoo Kim]

On Wed, Apr 23, 2014 at 10:32:30AM +0800, Weijie Yang wrote:
> On Wed, Apr 23, 2014 at 3:55 AM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Tue, 22 Apr 2014 11:14:02 +0800 Weijie Yang <weijie.yang@samsung.com> wrote:
> >
> >> we want to skip the logical block which is partially covered by
> >> the discard bio, so check the remaining size and subtract it if
> >> there is a need to goto the next logical block.
> >>
> >> This patch corrects the offset usage in zram_bio_discard.
> >>
> >
> > What were the end-user visible effects of the bug?
> >
> > Please always include this information when fixing something so that
> > others can work out which kernel(s) need patching.
> >
> 
> Thanks for your advise, I will resend this patch and add the end-user
> visible effect information.

Thanks for fixing it.

As far as I understand, there is no end-user visible effect, because
request size is alway PAGE_SIZE aligned and if n < PAGE_SIZE,
no real operation happens. Am I missing?

Anyway,

Acked-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Thanks.

Re: [PATCH] zram: correct offset usage in zram_bio_discard

[Weijie Yang]

On Wed, Apr 23, 2014 at 11:08 AM, Joonsoo Kim <iamjoonsoo.kim@lge.com> wrote:
> On Wed, Apr 23, 2014 at 10:32:30AM +0800, Weijie Yang wrote:
>> On Wed, Apr 23, 2014 at 3:55 AM, Andrew Morton
>> <akpm@linux-foundation.org> wrote:
>> > On Tue, 22 Apr 2014 11:14:02 +0800 Weijie Yang <weijie.yang@samsung.com> wrote:
>> >
>> >> we want to skip the logical block which is partially covered by
>> >> the discard bio, so check the remaining size and subtract it if
>> >> there is a need to goto the next logical block.
>> >>
>> >> This patch corrects the offset usage in zram_bio_discard.
>> >>
>> >
>> > What were the end-user visible effects of the bug?
>> >
>> > Please always include this information when fixing something so that
>> > others can work out which kernel(s) need patching.
>> >
>>
>> Thanks for your advise, I will resend this patch and add the end-user
>> visible effect information.
>
> Thanks for fixing it.
>
> As far as I understand, there is no end-user visible effect, because
> request size is alway PAGE_SIZE aligned and if n < PAGE_SIZE,
> no real operation happens. Am I missing?

The zram only limit ZRAM_LOGICAL_BLOCK_SIZE(4K) aligned,
not PAGE_SIZE aligned.

Consider the following scenario:
on some architecture or config, PAGE_SIZE is 64K for example
filesystem is set up on zram disk without PAGE_SIZE aligned.
a discard bio leads to a offset = 4K, size=72K
normally, it should not really discard any physical block as it partially
cover two physical blocks.
However, with the current offset usage, it will discard the second
physical block and free its memory, which will cause filesystem breakdown.

regards,

> Anyway,
>
> Acked-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>
> Thanks.

Re: [PATCH] zram: correct offset usage in zram_bio_discard

[Joonsoo Kim]

On Wed, Apr 23, 2014 at 11:52:08AM +0800, Weijie Yang wrote:
> On Wed, Apr 23, 2014 at 11:08 AM, Joonsoo Kim <iamjoonsoo.kim@lge.com> wrote:
> > On Wed, Apr 23, 2014 at 10:32:30AM +0800, Weijie Yang wrote:
> >> On Wed, Apr 23, 2014 at 3:55 AM, Andrew Morton
> >> <akpm@linux-foundation.org> wrote:
> >> > On Tue, 22 Apr 2014 11:14:02 +0800 Weijie Yang <weijie.yang@samsung.com> wrote:
> >> >
> >> >> we want to skip the logical block which is partially covered by
> >> >> the discard bio, so check the remaining size and subtract it if
> >> >> there is a need to goto the next logical block.
> >> >>
> >> >> This patch corrects the offset usage in zram_bio_discard.
> >> >>
> >> >
> >> > What were the end-user visible effects of the bug?
> >> >
> >> > Please always include this information when fixing something so that
> >> > others can work out which kernel(s) need patching.
> >> >
> >>
> >> Thanks for your advise, I will resend this patch and add the end-user
> >> visible effect information.
> >
> > Thanks for fixing it.
> >
> > As far as I understand, there is no end-user visible effect, because
> > request size is alway PAGE_SIZE aligned and if n < PAGE_SIZE,
> > no real operation happens. Am I missing?
> 
> The zram only limit ZRAM_LOGICAL_BLOCK_SIZE(4K) aligned,
> not PAGE_SIZE aligned.
> 
> Consider the following scenario:
> on some architecture or config, PAGE_SIZE is 64K for example
> filesystem is set up on zram disk without PAGE_SIZE aligned.
> a discard bio leads to a offset = 4K, size=72K
> normally, it should not really discard any physical block as it partially
> cover two physical blocks.
> However, with the current offset usage, it will discard the second
> physical block and free its memory, which will cause filesystem breakdown.
> 

I misunderstood what discard_granularity means. I thought that if I set
discard_granularity=PAGE_SIZE, the size of discard request is aligned to
PAGE_SIZE. But, by looking at the code, I notice that isn't true. So now, I
understand your patch compeletely. Thanks again.

Thanks.