Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Minchan Kim]

On Tue, Apr 18, 2017 at 10:53:10AM +0900, Sergey Senozhatsky wrote:
> Hello,
> 
> On (04/18/17 08:53), Minchan Kim wrote:
> > On Mon, Apr 17, 2017 at 07:50:16PM +0900, Sergey Senozhatsky wrote:
> > > Hello Minchan,
> > > 
> > > On (04/17/17 11:14), Minchan Kim wrote:
> > > > On Mon, Apr 17, 2017 at 10:54:29AM +0900, Sergey Senozhatsky wrote:
> > > > > On (04/17/17 10:21), Sergey Senozhatsky wrote:
> > > > > > > However, it should be *fixed* to prevent confusion in future
> > > > > 
> > > > > or may be something like below? can save us some cycles.
> > > > > 
> > > > > remove this calculation
> > > > > 
> > > > > -   offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> > > > > 
> > > > > 
> > > > > and pass 0 to zram_bvec_rw()
> > > > > 
> > > > > -   err = zram_bvec_rw(zram, , index, offset, is_write);
> > > > > +   err = zram_bvec_rw(zram, , index, 0, is_write);
> > > > 
> > > > That was one I wrote but have thought it more.
> > > > 
> > > > Because I suspect fs can submit page-size IO in non-aligned PAGE_SIZE
> > > > sector? For example, it can submit PAGE_SIZE read request from 9 sector.
> > > > Is it possible? I don't know.
> > > > 
> > > > As well, FS can format zram from sector 1, not sector 0? IOW, can't it
> > > > use starting sector as non-page algined sector?
> > > > We can do it via fdisk?
> > > > 
> > > > Anyway, If one of scenario I mentioned is possible, zram_rw_page will
> > > > be broken.
> > > > 
> > > > If it's hard to check all of scenario in this moment, it would be
> > > > better to not remove it and then add WARN_ON(offset) in there.
> > > > 
> > > > While I am writing this, I found this.
> > > > 
> > > > /**
> > > >  * bdev_read_page() - Start reading a page from a block device
> > > >  * @bdev: The device to read the page from
> > > >  * @sector: The offset on the device to read the page to (need not be 
> > > > aligned)
> > > >  * @page: The page to read
> > > >  *
> > > > 
> > > > Hmm,, need investigation but no time.
> > > 
> > > good questions.
> > > 
> > > as far as I can see, we never use 'offset' which we pass to zram_bvec_rw()
> > > from zram_rw_page(). `offset' makes a lot of sense for partial IO, but in
> > > zram_bvec_rw() we always do "bv.bv_len = PAGE_SIZE".
> > > 
> > > so what we have is
> > > 
> > > for READ
> > > 
> > > zram_rw_page()
> > >   bv.bv_len = PAGE_SIZE
> > >   zram_bvec_rw(zram, , index, offset, is_write);
> > >   zram_bvec_read()
> > >   if (is_partial_io(bvec))// always false
> > >   memcpy(user_mem + bvec->bv_offset,
> > >   uncmem + offset,
> > >   bvec->bv_len);
> > > 
> > > 
> > > for WRITE
> > > 
> > > zram_rw_page()
> > >   bv.bv_len = PAGE_SIZE
> > >   zram_bvec_rw(zram, , index, offset, is_write);
> > >   zram_bvec_write()
> > >   if (is_partial_io(bvec))// always false
> > >   memcpy(uncmem + offset,
> > >   user_mem + bvec->bv_offset,
> > >   bvec->bv_len);
> > > 
> > > 
> > > and our is_partial_io() looks at ->bv_len:
> > > 
> > >   bvec->bv_len != PAGE_SIZE;
> > > 
> > > which we set to PAGE_SIZE.
> > > 
> > > so in the existing scheme of things, we never care about 'sector'
> > > passed from zram_rw_page(). and this has worked for us for quite
> > > some time. my call would be -- let's drop zram_rw_page() `sector'
> > > calculation.
> > 
> > I can do but before that, I want to confirm. Ccing Matthew,
> > Summary for Matthew,
> > 
> > I see following comment about the sector from bdev_read_page.
> > 
> > /**
> >  * bdev_read_page() - Start reading a page from a block device
> >  * @bdev: The device to read the page from
> >  * @sector: The offset on the device to read the page to (need not be 
> > aligned)
> >  * @page: The page to read
> >  *
> > 
> > Does it mean that sector can be not aligned PAGE_SIZE?
> > 
> > For example, 512byte sector, 4K page system, 4K = 8 sector
> > 
> > bdev_read_page(bdev, 9, page);
> 
> do you mean a sector that spans two pages? sectors are pow of 2 in size
> and pages are pow of 2 in size, so page_size is `K * sector_size', isn't
> it?
> 
> fs/mpage.c
> 
> static struct bio *
> do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
> sector_t *last_block_in_bio, struct buffer_head *map_bh,
> unsigned long *first_logical_block, get_block_t get_block,
> gfp_t gfp)
> {
> const unsigned blkbits = inode->i_blkbits;
> const unsigned blocks_per_page = PAGE_SIZE >> blkbits;
> const unsigned blocksize = 1 << blkbits;
> sector_t block_in_file;
> sector_t last_block;
> sector_t last_block_in_file;
> sector_t blocks[MAX_BUF_PER_PAGE];
>   ...
> block_in_file = (sector_t)page->index << (PAGE_SHIFT - 

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Minchan Kim]

On Tue, Apr 18, 2017 at 10:53:10AM +0900, Sergey Senozhatsky wrote:
> Hello,
> 
> On (04/18/17 08:53), Minchan Kim wrote:
> > On Mon, Apr 17, 2017 at 07:50:16PM +0900, Sergey Senozhatsky wrote:
> > > Hello Minchan,
> > > 
> > > On (04/17/17 11:14), Minchan Kim wrote:
> > > > On Mon, Apr 17, 2017 at 10:54:29AM +0900, Sergey Senozhatsky wrote:
> > > > > On (04/17/17 10:21), Sergey Senozhatsky wrote:
> > > > > > > However, it should be *fixed* to prevent confusion in future
> > > > > 
> > > > > or may be something like below? can save us some cycles.
> > > > > 
> > > > > remove this calculation
> > > > > 
> > > > > -   offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> > > > > 
> > > > > 
> > > > > and pass 0 to zram_bvec_rw()
> > > > > 
> > > > > -   err = zram_bvec_rw(zram, , index, offset, is_write);
> > > > > +   err = zram_bvec_rw(zram, , index, 0, is_write);
> > > > 
> > > > That was one I wrote but have thought it more.
> > > > 
> > > > Because I suspect fs can submit page-size IO in non-aligned PAGE_SIZE
> > > > sector? For example, it can submit PAGE_SIZE read request from 9 sector.
> > > > Is it possible? I don't know.
> > > > 
> > > > As well, FS can format zram from sector 1, not sector 0? IOW, can't it
> > > > use starting sector as non-page algined sector?
> > > > We can do it via fdisk?
> > > > 
> > > > Anyway, If one of scenario I mentioned is possible, zram_rw_page will
> > > > be broken.
> > > > 
> > > > If it's hard to check all of scenario in this moment, it would be
> > > > better to not remove it and then add WARN_ON(offset) in there.
> > > > 
> > > > While I am writing this, I found this.
> > > > 
> > > > /**
> > > >  * bdev_read_page() - Start reading a page from a block device
> > > >  * @bdev: The device to read the page from
> > > >  * @sector: The offset on the device to read the page to (need not be 
> > > > aligned)
> > > >  * @page: The page to read
> > > >  *
> > > > 
> > > > Hmm,, need investigation but no time.
> > > 
> > > good questions.
> > > 
> > > as far as I can see, we never use 'offset' which we pass to zram_bvec_rw()
> > > from zram_rw_page(). `offset' makes a lot of sense for partial IO, but in
> > > zram_bvec_rw() we always do "bv.bv_len = PAGE_SIZE".
> > > 
> > > so what we have is
> > > 
> > > for READ
> > > 
> > > zram_rw_page()
> > >   bv.bv_len = PAGE_SIZE
> > >   zram_bvec_rw(zram, , index, offset, is_write);
> > >   zram_bvec_read()
> > >   if (is_partial_io(bvec))// always false
> > >   memcpy(user_mem + bvec->bv_offset,
> > >   uncmem + offset,
> > >   bvec->bv_len);
> > > 
> > > 
> > > for WRITE
> > > 
> > > zram_rw_page()
> > >   bv.bv_len = PAGE_SIZE
> > >   zram_bvec_rw(zram, , index, offset, is_write);
> > >   zram_bvec_write()
> > >   if (is_partial_io(bvec))// always false
> > >   memcpy(uncmem + offset,
> > >   user_mem + bvec->bv_offset,
> > >   bvec->bv_len);
> > > 
> > > 
> > > and our is_partial_io() looks at ->bv_len:
> > > 
> > >   bvec->bv_len != PAGE_SIZE;
> > > 
> > > which we set to PAGE_SIZE.
> > > 
> > > so in the existing scheme of things, we never care about 'sector'
> > > passed from zram_rw_page(). and this has worked for us for quite
> > > some time. my call would be -- let's drop zram_rw_page() `sector'
> > > calculation.
> > 
> > I can do but before that, I want to confirm. Ccing Matthew,
> > Summary for Matthew,
> > 
> > I see following comment about the sector from bdev_read_page.
> > 
> > /**
> >  * bdev_read_page() - Start reading a page from a block device
> >  * @bdev: The device to read the page from
> >  * @sector: The offset on the device to read the page to (need not be 
> > aligned)
> >  * @page: The page to read
> >  *
> > 
> > Does it mean that sector can be not aligned PAGE_SIZE?
> > 
> > For example, 512byte sector, 4K page system, 4K = 8 sector
> > 
> > bdev_read_page(bdev, 9, page);
> 
> do you mean a sector that spans two pages? sectors are pow of 2 in size
> and pages are pow of 2 in size, so page_size is `K * sector_size', isn't
> it?
> 
> fs/mpage.c
> 
> static struct bio *
> do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
> sector_t *last_block_in_bio, struct buffer_head *map_bh,
> unsigned long *first_logical_block, get_block_t get_block,
> gfp_t gfp)
> {
> const unsigned blkbits = inode->i_blkbits;
> const unsigned blocks_per_page = PAGE_SIZE >> blkbits;
> const unsigned blocksize = 1 << blkbits;
> sector_t block_in_file;
> sector_t last_block;
> sector_t last_block_in_file;
> sector_t blocks[MAX_BUF_PER_PAGE];
>   ...
> block_in_file = (sector_t)page->index << (PAGE_SHIFT - 

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Sergey Senozhatsky]

Hello,

On (04/18/17 08:53), Minchan Kim wrote:
> On Mon, Apr 17, 2017 at 07:50:16PM +0900, Sergey Senozhatsky wrote:
> > Hello Minchan,
> > 
> > On (04/17/17 11:14), Minchan Kim wrote:
> > > On Mon, Apr 17, 2017 at 10:54:29AM +0900, Sergey Senozhatsky wrote:
> > > > On (04/17/17 10:21), Sergey Senozhatsky wrote:
> > > > > > However, it should be *fixed* to prevent confusion in future
> > > > 
> > > > or may be something like below? can save us some cycles.
> > > > 
> > > > remove this calculation
> > > > 
> > > > -   offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> > > > 
> > > > 
> > > > and pass 0 to zram_bvec_rw()
> > > > 
> > > > -   err = zram_bvec_rw(zram, , index, offset, is_write);
> > > > +   err = zram_bvec_rw(zram, , index, 0, is_write);
> > > 
> > > That was one I wrote but have thought it more.
> > > 
> > > Because I suspect fs can submit page-size IO in non-aligned PAGE_SIZE
> > > sector? For example, it can submit PAGE_SIZE read request from 9 sector.
> > > Is it possible? I don't know.
> > > 
> > > As well, FS can format zram from sector 1, not sector 0? IOW, can't it
> > > use starting sector as non-page algined sector?
> > > We can do it via fdisk?
> > > 
> > > Anyway, If one of scenario I mentioned is possible, zram_rw_page will
> > > be broken.
> > > 
> > > If it's hard to check all of scenario in this moment, it would be
> > > better to not remove it and then add WARN_ON(offset) in there.
> > > 
> > > While I am writing this, I found this.
> > > 
> > > /**
> > >  * bdev_read_page() - Start reading a page from a block device
> > >  * @bdev: The device to read the page from
> > >  * @sector: The offset on the device to read the page to (need not be 
> > > aligned)
> > >  * @page: The page to read
> > >  *
> > > 
> > > Hmm,, need investigation but no time.
> > 
> > good questions.
> > 
> > as far as I can see, we never use 'offset' which we pass to zram_bvec_rw()
> > from zram_rw_page(). `offset' makes a lot of sense for partial IO, but in
> > zram_bvec_rw() we always do "bv.bv_len = PAGE_SIZE".
> > 
> > so what we have is
> > 
> > for READ
> > 
> > zram_rw_page()
> > bv.bv_len = PAGE_SIZE
> > zram_bvec_rw(zram, , index, offset, is_write);
> > zram_bvec_read()
> > if (is_partial_io(bvec))// always false
> > memcpy(user_mem + bvec->bv_offset,
> > uncmem + offset,
> > bvec->bv_len);
> > 
> > 
> > for WRITE
> > 
> > zram_rw_page()
> > bv.bv_len = PAGE_SIZE
> > zram_bvec_rw(zram, , index, offset, is_write);
> > zram_bvec_write()
> > if (is_partial_io(bvec))// always false
> > memcpy(uncmem + offset,
> > user_mem + bvec->bv_offset,
> > bvec->bv_len);
> > 
> > 
> > and our is_partial_io() looks at ->bv_len:
> > 
> > bvec->bv_len != PAGE_SIZE;
> > 
> > which we set to PAGE_SIZE.
> > 
> > so in the existing scheme of things, we never care about 'sector'
> > passed from zram_rw_page(). and this has worked for us for quite
> > some time. my call would be -- let's drop zram_rw_page() `sector'
> > calculation.
> 
> I can do but before that, I want to confirm. Ccing Matthew,
> Summary for Matthew,
> 
> I see following comment about the sector from bdev_read_page.
> 
> /**
>  * bdev_read_page() - Start reading a page from a block device
>  * @bdev: The device to read the page from
>  * @sector: The offset on the device to read the page to (need not be aligned)
>  * @page: The page to read
>  *
> 
> Does it mean that sector can be not aligned PAGE_SIZE?
> 
> For example, 512byte sector, 4K page system, 4K = 8 sector
> 
> bdev_read_page(bdev, 9, page);

do you mean a sector that spans two pages? sectors are pow of 2 in size
and pages are pow of 2 in size, so page_size is `K * sector_size', isn't
it?

fs/mpage.c

static struct bio *
do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
sector_t *last_block_in_bio, struct buffer_head *map_bh,
unsigned long *first_logical_block, get_block_t get_block,
gfp_t gfp)
{
const unsigned blkbits = inode->i_blkbits;
const unsigned blocks_per_page = PAGE_SIZE >> blkbits;
const unsigned blocksize = 1 << blkbits;
sector_t block_in_file;
sector_t last_block;
sector_t last_block_in_file;
sector_t blocks[MAX_BUF_PER_PAGE];
...
block_in_file = (sector_t)page->index << (PAGE_SHIFT - blkbits);
last_block = block_in_file + nr_pages * blocks_per_page;
last_block_in_file = (i_size_read(inode) + blocksize - 1) >> blkbits;
if (last_block > last_block_in_file)
last_block = last_block_in_file;

or did I misunderstood your question?

-ss

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Sergey Senozhatsky]

Hello,

On (04/18/17 08:53), Minchan Kim wrote:
> On Mon, Apr 17, 2017 at 07:50:16PM +0900, Sergey Senozhatsky wrote:
> > Hello Minchan,
> > 
> > On (04/17/17 11:14), Minchan Kim wrote:
> > > On Mon, Apr 17, 2017 at 10:54:29AM +0900, Sergey Senozhatsky wrote:
> > > > On (04/17/17 10:21), Sergey Senozhatsky wrote:
> > > > > > However, it should be *fixed* to prevent confusion in future
> > > > 
> > > > or may be something like below? can save us some cycles.
> > > > 
> > > > remove this calculation
> > > > 
> > > > -   offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> > > > 
> > > > 
> > > > and pass 0 to zram_bvec_rw()
> > > > 
> > > > -   err = zram_bvec_rw(zram, , index, offset, is_write);
> > > > +   err = zram_bvec_rw(zram, , index, 0, is_write);
> > > 
> > > That was one I wrote but have thought it more.
> > > 
> > > Because I suspect fs can submit page-size IO in non-aligned PAGE_SIZE
> > > sector? For example, it can submit PAGE_SIZE read request from 9 sector.
> > > Is it possible? I don't know.
> > > 
> > > As well, FS can format zram from sector 1, not sector 0? IOW, can't it
> > > use starting sector as non-page algined sector?
> > > We can do it via fdisk?
> > > 
> > > Anyway, If one of scenario I mentioned is possible, zram_rw_page will
> > > be broken.
> > > 
> > > If it's hard to check all of scenario in this moment, it would be
> > > better to not remove it and then add WARN_ON(offset) in there.
> > > 
> > > While I am writing this, I found this.
> > > 
> > > /**
> > >  * bdev_read_page() - Start reading a page from a block device
> > >  * @bdev: The device to read the page from
> > >  * @sector: The offset on the device to read the page to (need not be 
> > > aligned)
> > >  * @page: The page to read
> > >  *
> > > 
> > > Hmm,, need investigation but no time.
> > 
> > good questions.
> > 
> > as far as I can see, we never use 'offset' which we pass to zram_bvec_rw()
> > from zram_rw_page(). `offset' makes a lot of sense for partial IO, but in
> > zram_bvec_rw() we always do "bv.bv_len = PAGE_SIZE".
> > 
> > so what we have is
> > 
> > for READ
> > 
> > zram_rw_page()
> > bv.bv_len = PAGE_SIZE
> > zram_bvec_rw(zram, , index, offset, is_write);
> > zram_bvec_read()
> > if (is_partial_io(bvec))// always false
> > memcpy(user_mem + bvec->bv_offset,
> > uncmem + offset,
> > bvec->bv_len);
> > 
> > 
> > for WRITE
> > 
> > zram_rw_page()
> > bv.bv_len = PAGE_SIZE
> > zram_bvec_rw(zram, , index, offset, is_write);
> > zram_bvec_write()
> > if (is_partial_io(bvec))// always false
> > memcpy(uncmem + offset,
> > user_mem + bvec->bv_offset,
> > bvec->bv_len);
> > 
> > 
> > and our is_partial_io() looks at ->bv_len:
> > 
> > bvec->bv_len != PAGE_SIZE;
> > 
> > which we set to PAGE_SIZE.
> > 
> > so in the existing scheme of things, we never care about 'sector'
> > passed from zram_rw_page(). and this has worked for us for quite
> > some time. my call would be -- let's drop zram_rw_page() `sector'
> > calculation.
> 
> I can do but before that, I want to confirm. Ccing Matthew,
> Summary for Matthew,
> 
> I see following comment about the sector from bdev_read_page.
> 
> /**
>  * bdev_read_page() - Start reading a page from a block device
>  * @bdev: The device to read the page from
>  * @sector: The offset on the device to read the page to (need not be aligned)
>  * @page: The page to read
>  *
> 
> Does it mean that sector can be not aligned PAGE_SIZE?
> 
> For example, 512byte sector, 4K page system, 4K = 8 sector
> 
> bdev_read_page(bdev, 9, page);

do you mean a sector that spans two pages? sectors are pow of 2 in size
and pages are pow of 2 in size, so page_size is `K * sector_size', isn't
it?

fs/mpage.c

static struct bio *
do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
sector_t *last_block_in_bio, struct buffer_head *map_bh,
unsigned long *first_logical_block, get_block_t get_block,
gfp_t gfp)
{
const unsigned blkbits = inode->i_blkbits;
const unsigned blocks_per_page = PAGE_SIZE >> blkbits;
const unsigned blocksize = 1 << blkbits;
sector_t block_in_file;
sector_t last_block;
sector_t last_block_in_file;
sector_t blocks[MAX_BUF_PER_PAGE];
...
block_in_file = (sector_t)page->index << (PAGE_SHIFT - blkbits);
last_block = block_in_file + nr_pages * blocks_per_page;
last_block_in_file = (i_size_read(inode) + blocksize - 1) >> blkbits;
if (last_block > last_block_in_file)
last_block = last_block_in_file;

or did I misunderstood your question?

-ss

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Minchan Kim]

Hi Sergey,

On Mon, Apr 17, 2017 at 07:50:16PM +0900, Sergey Senozhatsky wrote:
> Hello Minchan,
> 
> On (04/17/17 11:14), Minchan Kim wrote:
> > On Mon, Apr 17, 2017 at 10:54:29AM +0900, Sergey Senozhatsky wrote:
> > > On (04/17/17 10:21), Sergey Senozhatsky wrote:
> > > > > However, it should be *fixed* to prevent confusion in future
> > > 
> > > or may be something like below? can save us some cycles.
> > > 
> > > remove this calculation
> > > 
> > > -   offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> > > 
> > > 
> > > and pass 0 to zram_bvec_rw()
> > > 
> > > -   err = zram_bvec_rw(zram, , index, offset, is_write);
> > > +   err = zram_bvec_rw(zram, , index, 0, is_write);
> > 
> > That was one I wrote but have thought it more.
> > 
> > Because I suspect fs can submit page-size IO in non-aligned PAGE_SIZE
> > sector? For example, it can submit PAGE_SIZE read request from 9 sector.
> > Is it possible? I don't know.
> > 
> > As well, FS can format zram from sector 1, not sector 0? IOW, can't it
> > use starting sector as non-page algined sector?
> > We can do it via fdisk?
> > 
> > Anyway, If one of scenario I mentioned is possible, zram_rw_page will
> > be broken.
> > 
> > If it's hard to check all of scenario in this moment, it would be
> > better to not remove it and then add WARN_ON(offset) in there.
> > 
> > While I am writing this, I found this.
> > 
> > /**
> >  * bdev_read_page() - Start reading a page from a block device
> >  * @bdev: The device to read the page from
> >  * @sector: The offset on the device to read the page to (need not be 
> > aligned)
> >  * @page: The page to read
> >  *
> > 
> > Hmm,, need investigation but no time.
> 
> good questions.
> 
> as far as I can see, we never use 'offset' which we pass to zram_bvec_rw()
> from zram_rw_page(). `offset' makes a lot of sense for partial IO, but in
> zram_bvec_rw() we always do "bv.bv_len = PAGE_SIZE".
> 
> so what we have is
> 
> for READ
> 
> zram_rw_page()
>   bv.bv_len = PAGE_SIZE
>   zram_bvec_rw(zram, , index, offset, is_write);
>   zram_bvec_read()
>   if (is_partial_io(bvec))// always false
>   memcpy(user_mem + bvec->bv_offset,
>   uncmem + offset,
>   bvec->bv_len);
> 
> 
> for WRITE
> 
> zram_rw_page()
>   bv.bv_len = PAGE_SIZE
>   zram_bvec_rw(zram, , index, offset, is_write);
>   zram_bvec_write()
>   if (is_partial_io(bvec))// always false
>   memcpy(uncmem + offset,
>   user_mem + bvec->bv_offset,
>   bvec->bv_len);
> 
> 
> and our is_partial_io() looks at ->bv_len:
> 
>   bvec->bv_len != PAGE_SIZE;
> 
> which we set to PAGE_SIZE.
> 
> so in the existing scheme of things, we never care about 'sector'
> passed from zram_rw_page(). and this has worked for us for quite
> some time. my call would be -- let's drop zram_rw_page() `sector'
> calculation.

I can do but before that, I want to confirm. Ccing Matthew,
Summary for Matthew,

I see following comment about the sector from bdev_read_page.

/**
 * bdev_read_page() - Start reading a page from a block device
 * @bdev: The device to read the page from
 * @sector: The offset on the device to read the page to (need not be aligned)
 * @page: The page to read
 *

Does it mean that sector can be not aligned PAGE_SIZE?

For example, 512byte sector, 4K page system, 4K = 8 sector

bdev_read_page(bdev, 9, page);

is possible for driver declared below?

blk_queue_physical_block_size(zram->disk->queue, PAGE_SIZE);
blk_queue_logical_block_size(zram->disk->queue,
ZRAM_LOGICAL_BLOCK_SIZE);

ZRAM_LOGICAL_BLOCK_SIZE is 4K regradless of 4K/64K page architecure.

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Minchan Kim]

Hi Sergey,

On Mon, Apr 17, 2017 at 07:50:16PM +0900, Sergey Senozhatsky wrote:
> Hello Minchan,
> 
> On (04/17/17 11:14), Minchan Kim wrote:
> > On Mon, Apr 17, 2017 at 10:54:29AM +0900, Sergey Senozhatsky wrote:
> > > On (04/17/17 10:21), Sergey Senozhatsky wrote:
> > > > > However, it should be *fixed* to prevent confusion in future
> > > 
> > > or may be something like below? can save us some cycles.
> > > 
> > > remove this calculation
> > > 
> > > -   offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> > > 
> > > 
> > > and pass 0 to zram_bvec_rw()
> > > 
> > > -   err = zram_bvec_rw(zram, , index, offset, is_write);
> > > +   err = zram_bvec_rw(zram, , index, 0, is_write);
> > 
> > That was one I wrote but have thought it more.
> > 
> > Because I suspect fs can submit page-size IO in non-aligned PAGE_SIZE
> > sector? For example, it can submit PAGE_SIZE read request from 9 sector.
> > Is it possible? I don't know.
> > 
> > As well, FS can format zram from sector 1, not sector 0? IOW, can't it
> > use starting sector as non-page algined sector?
> > We can do it via fdisk?
> > 
> > Anyway, If one of scenario I mentioned is possible, zram_rw_page will
> > be broken.
> > 
> > If it's hard to check all of scenario in this moment, it would be
> > better to not remove it and then add WARN_ON(offset) in there.
> > 
> > While I am writing this, I found this.
> > 
> > /**
> >  * bdev_read_page() - Start reading a page from a block device
> >  * @bdev: The device to read the page from
> >  * @sector: The offset on the device to read the page to (need not be 
> > aligned)
> >  * @page: The page to read
> >  *
> > 
> > Hmm,, need investigation but no time.
> 
> good questions.
> 
> as far as I can see, we never use 'offset' which we pass to zram_bvec_rw()
> from zram_rw_page(). `offset' makes a lot of sense for partial IO, but in
> zram_bvec_rw() we always do "bv.bv_len = PAGE_SIZE".
> 
> so what we have is
> 
> for READ
> 
> zram_rw_page()
>   bv.bv_len = PAGE_SIZE
>   zram_bvec_rw(zram, , index, offset, is_write);
>   zram_bvec_read()
>   if (is_partial_io(bvec))// always false
>   memcpy(user_mem + bvec->bv_offset,
>   uncmem + offset,
>   bvec->bv_len);
> 
> 
> for WRITE
> 
> zram_rw_page()
>   bv.bv_len = PAGE_SIZE
>   zram_bvec_rw(zram, , index, offset, is_write);
>   zram_bvec_write()
>   if (is_partial_io(bvec))// always false
>   memcpy(uncmem + offset,
>   user_mem + bvec->bv_offset,
>   bvec->bv_len);
> 
> 
> and our is_partial_io() looks at ->bv_len:
> 
>   bvec->bv_len != PAGE_SIZE;
> 
> which we set to PAGE_SIZE.
> 
> so in the existing scheme of things, we never care about 'sector'
> passed from zram_rw_page(). and this has worked for us for quite
> some time. my call would be -- let's drop zram_rw_page() `sector'
> calculation.

I can do but before that, I want to confirm. Ccing Matthew,
Summary for Matthew,

I see following comment about the sector from bdev_read_page.

/**
 * bdev_read_page() - Start reading a page from a block device
 * @bdev: The device to read the page from
 * @sector: The offset on the device to read the page to (need not be aligned)
 * @page: The page to read
 *

Does it mean that sector can be not aligned PAGE_SIZE?

For example, 512byte sector, 4K page system, 4K = 8 sector

bdev_read_page(bdev, 9, page);

is possible for driver declared below?

blk_queue_physical_block_size(zram->disk->queue, PAGE_SIZE);
blk_queue_logical_block_size(zram->disk->queue,
ZRAM_LOGICAL_BLOCK_SIZE);

ZRAM_LOGICAL_BLOCK_SIZE is 4K regradless of 4K/64K page architecure.

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Sergey Senozhatsky]

On (04/17/17 19:50), Sergey Senozhatsky wrote:
[..]
> so in the existing scheme of things, we never care about 'sector'
> passed from zram_rw_page(). and this has worked for us for quite
> some time. my call would be -- let's drop zram_rw_page() `sector'
> calculation.

d'oh... s/sector/offset/g


-ss

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Sergey Senozhatsky]

On (04/17/17 19:50), Sergey Senozhatsky wrote:
[..]
> so in the existing scheme of things, we never care about 'sector'
> passed from zram_rw_page(). and this has worked for us for quite
> some time. my call would be -- let's drop zram_rw_page() `sector'
> calculation.

d'oh... s/sector/offset/g


-ss

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Sergey Senozhatsky]

Hello Minchan,

On (04/17/17 11:14), Minchan Kim wrote:
> On Mon, Apr 17, 2017 at 10:54:29AM +0900, Sergey Senozhatsky wrote:
> > On (04/17/17 10:21), Sergey Senozhatsky wrote:
> > > > However, it should be *fixed* to prevent confusion in future
> > 
> > or may be something like below? can save us some cycles.
> > 
> > remove this calculation
> > 
> > -   offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> > 
> > 
> > and pass 0 to zram_bvec_rw()
> > 
> > -   err = zram_bvec_rw(zram, , index, offset, is_write);
> > +   err = zram_bvec_rw(zram, , index, 0, is_write);
> 
> That was one I wrote but have thought it more.
> 
> Because I suspect fs can submit page-size IO in non-aligned PAGE_SIZE
> sector? For example, it can submit PAGE_SIZE read request from 9 sector.
> Is it possible? I don't know.
> 
> As well, FS can format zram from sector 1, not sector 0? IOW, can't it
> use starting sector as non-page algined sector?
> We can do it via fdisk?
> 
> Anyway, If one of scenario I mentioned is possible, zram_rw_page will
> be broken.
> 
> If it's hard to check all of scenario in this moment, it would be
> better to not remove it and then add WARN_ON(offset) in there.
> 
> While I am writing this, I found this.
> 
> /**
>  * bdev_read_page() - Start reading a page from a block device
>  * @bdev: The device to read the page from
>  * @sector: The offset on the device to read the page to (need not be aligned)
>  * @page: The page to read
>  *
> 
> Hmm,, need investigation but no time.

good questions.

as far as I can see, we never use 'offset' which we pass to zram_bvec_rw()
from zram_rw_page(). `offset' makes a lot of sense for partial IO, but in
zram_bvec_rw() we always do "bv.bv_len = PAGE_SIZE".

so what we have is

for READ

zram_rw_page()
bv.bv_len = PAGE_SIZE
zram_bvec_rw(zram, , index, offset, is_write);
zram_bvec_read()
if (is_partial_io(bvec))// always false
memcpy(user_mem + bvec->bv_offset,
uncmem + offset,
bvec->bv_len);


for WRITE

zram_rw_page()
bv.bv_len = PAGE_SIZE
zram_bvec_rw(zram, , index, offset, is_write);
zram_bvec_write()
if (is_partial_io(bvec))// always false
memcpy(uncmem + offset,
user_mem + bvec->bv_offset,
bvec->bv_len);


and our is_partial_io() looks at ->bv_len:

bvec->bv_len != PAGE_SIZE;

which we set to PAGE_SIZE.

so in the existing scheme of things, we never care about 'sector'
passed from zram_rw_page(). and this has worked for us for quite
some time. my call would be -- let's drop zram_rw_page() `sector'
calculation.

-ss

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Sergey Senozhatsky]

Hello Minchan,

On (04/17/17 11:14), Minchan Kim wrote:
> On Mon, Apr 17, 2017 at 10:54:29AM +0900, Sergey Senozhatsky wrote:
> > On (04/17/17 10:21), Sergey Senozhatsky wrote:
> > > > However, it should be *fixed* to prevent confusion in future
> > 
> > or may be something like below? can save us some cycles.
> > 
> > remove this calculation
> > 
> > -   offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> > 
> > 
> > and pass 0 to zram_bvec_rw()
> > 
> > -   err = zram_bvec_rw(zram, , index, offset, is_write);
> > +   err = zram_bvec_rw(zram, , index, 0, is_write);
> 
> That was one I wrote but have thought it more.
> 
> Because I suspect fs can submit page-size IO in non-aligned PAGE_SIZE
> sector? For example, it can submit PAGE_SIZE read request from 9 sector.
> Is it possible? I don't know.
> 
> As well, FS can format zram from sector 1, not sector 0? IOW, can't it
> use starting sector as non-page algined sector?
> We can do it via fdisk?
> 
> Anyway, If one of scenario I mentioned is possible, zram_rw_page will
> be broken.
> 
> If it's hard to check all of scenario in this moment, it would be
> better to not remove it and then add WARN_ON(offset) in there.
> 
> While I am writing this, I found this.
> 
> /**
>  * bdev_read_page() - Start reading a page from a block device
>  * @bdev: The device to read the page from
>  * @sector: The offset on the device to read the page to (need not be aligned)
>  * @page: The page to read
>  *
> 
> Hmm,, need investigation but no time.

good questions.

as far as I can see, we never use 'offset' which we pass to zram_bvec_rw()
from zram_rw_page(). `offset' makes a lot of sense for partial IO, but in
zram_bvec_rw() we always do "bv.bv_len = PAGE_SIZE".

so what we have is

for READ

zram_rw_page()
bv.bv_len = PAGE_SIZE
zram_bvec_rw(zram, , index, offset, is_write);
zram_bvec_read()
if (is_partial_io(bvec))// always false
memcpy(user_mem + bvec->bv_offset,
uncmem + offset,
bvec->bv_len);


for WRITE

zram_rw_page()
bv.bv_len = PAGE_SIZE
zram_bvec_rw(zram, , index, offset, is_write);
zram_bvec_write()
if (is_partial_io(bvec))// always false
memcpy(uncmem + offset,
user_mem + bvec->bv_offset,
bvec->bv_len);


and our is_partial_io() looks at ->bv_len:

bvec->bv_len != PAGE_SIZE;

which we set to PAGE_SIZE.

so in the existing scheme of things, we never care about 'sector'
passed from zram_rw_page(). and this has worked for us for quite
some time. my call would be -- let's drop zram_rw_page() `sector'
calculation.

-ss

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Minchan Kim]

Hi Sergey,

On Mon, Apr 17, 2017 at 10:54:29AM +0900, Sergey Senozhatsky wrote:
> On (04/17/17 10:21), Sergey Senozhatsky wrote:
> > > However, it should be *fixed* to prevent confusion in future
> 
> or may be something like below? can save us some cycles.
> 
> remove this calculation
> 
> -   offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> 
> 
> and pass 0 to zram_bvec_rw()
> 
> -   err = zram_bvec_rw(zram, , index, offset, is_write);
> +   err = zram_bvec_rw(zram, , index, 0, is_write);

That was one I wrote but have thought it more.

Because I suspect fs can submit page-size IO in non-aligned PAGE_SIZE
sector? For example, it can submit PAGE_SIZE read request from 9 sector.
Is it possible? I don't know.

As well, FS can format zram from sector 1, not sector 0? IOW, can't it
use starting sector as non-page algined sector?
We can do it via fdisk?

Anyway, If one of scenario I mentioned is possible, zram_rw_page will
be broken.

If it's hard to check all of scenario in this moment, it would be
better to not remove it and then add WARN_ON(offset) in there.

While I am writing this, I found this.

/**
 * bdev_read_page() - Start reading a page from a block device
 * @bdev: The device to read the page from
 * @sector: The offset on the device to read the page to (need not be aligned)
 * @page: The page to read
 *

Hmm,, need investigation but no time.

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Minchan Kim]

Hi Sergey,

On Mon, Apr 17, 2017 at 10:54:29AM +0900, Sergey Senozhatsky wrote:
> On (04/17/17 10:21), Sergey Senozhatsky wrote:
> > > However, it should be *fixed* to prevent confusion in future
> 
> or may be something like below? can save us some cycles.
> 
> remove this calculation
> 
> -   offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> 
> 
> and pass 0 to zram_bvec_rw()
> 
> -   err = zram_bvec_rw(zram, , index, offset, is_write);
> +   err = zram_bvec_rw(zram, , index, 0, is_write);

That was one I wrote but have thought it more.

Because I suspect fs can submit page-size IO in non-aligned PAGE_SIZE
sector? For example, it can submit PAGE_SIZE read request from 9 sector.
Is it possible? I don't know.

As well, FS can format zram from sector 1, not sector 0? IOW, can't it
use starting sector as non-page algined sector?
We can do it via fdisk?

Anyway, If one of scenario I mentioned is possible, zram_rw_page will
be broken.

If it's hard to check all of scenario in this moment, it would be
better to not remove it and then add WARN_ON(offset) in there.

While I am writing this, I found this.

/**
 * bdev_read_page() - Start reading a page from a block device
 * @bdev: The device to read the page from
 * @sector: The offset on the device to read the page to (need not be aligned)
 * @page: The page to read
 *

Hmm,, need investigation but no time.

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Sergey Senozhatsky]

On (04/17/17 10:21), Sergey Senozhatsky wrote:
> > However, it should be *fixed* to prevent confusion in future

or may be something like below? can save us some cycles.

remove this calculation

-   offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;


and pass 0 to zram_bvec_rw()

-   err = zram_bvec_rw(zram, , index, offset, is_write);
+   err = zram_bvec_rw(zram, , index, 0, is_write);


-ss

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Sergey Senozhatsky]

On (04/17/17 10:21), Sergey Senozhatsky wrote:
> > However, it should be *fixed* to prevent confusion in future

or may be something like below? can save us some cycles.

remove this calculation

-   offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;


and pass 0 to zram_bvec_rw()

-   err = zram_bvec_rw(zram, , index, offset, is_write);
+   err = zram_bvec_rw(zram, , index, 0, is_write);


-ss

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Sergey Senozhatsky]

Hello,

On (04/15/17 00:33), Minchan Kim wrote:
> On Fri, Apr 14, 2017 at 02:07:47PM +0900, Sergey Senozhatsky wrote:
> > On (04/13/17 09:17), Minchan Kim wrote:
> > [..]
> > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > > index 9e2199060040..83c38a123242 100644
> > > --- a/drivers/block/zram/zram_drv.c
> > > +++ b/drivers/block/zram/zram_drv.c
> > > @@ -930,7 +930,7 @@ static int zram_rw_page(struct block_device *bdev, 
> > > sector_t sector,
> > >   }
> > >  
> > >   index = sector >> SECTORS_PER_PAGE_SHIFT;
> > > - offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> > > + offset = (sector & (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;
> > 
> > sorry, can it actually produce different results?
> 
> I got your point. Actually, offset was wrong but rw_page is called
> with PAGE_SIZE io while that offset is related to only partial io
> (non-PAGEE size io). IOW, although the wrong offset it is never used
> in functions.
> 
> To find subtle corruption in ppc64, I added some debug code to
> catch up wrong buffer overflow and found it with other bugs but
> didn't prove the specific case is valid case or not. Good catch, Sergey!
> 
> However, it should be *fixed* to prevent confusion in future but surely,
> no need to go to the stable. I will send reply to Greg to prevent merging
> it to *stable* when he send review asking to merge.

cool. thanks!

> And next week I will send another fix which *maybe* removes code to get the
> offset in zram_rw_page.

sounds interesting!

-ss

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Sergey Senozhatsky]

Hello,

On (04/15/17 00:33), Minchan Kim wrote:
> On Fri, Apr 14, 2017 at 02:07:47PM +0900, Sergey Senozhatsky wrote:
> > On (04/13/17 09:17), Minchan Kim wrote:
> > [..]
> > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > > index 9e2199060040..83c38a123242 100644
> > > --- a/drivers/block/zram/zram_drv.c
> > > +++ b/drivers/block/zram/zram_drv.c
> > > @@ -930,7 +930,7 @@ static int zram_rw_page(struct block_device *bdev, 
> > > sector_t sector,
> > >   }
> > >  
> > >   index = sector >> SECTORS_PER_PAGE_SHIFT;
> > > - offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> > > + offset = (sector & (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;
> > 
> > sorry, can it actually produce different results?
> 
> I got your point. Actually, offset was wrong but rw_page is called
> with PAGE_SIZE io while that offset is related to only partial io
> (non-PAGEE size io). IOW, although the wrong offset it is never used
> in functions.
> 
> To find subtle corruption in ppc64, I added some debug code to
> catch up wrong buffer overflow and found it with other bugs but
> didn't prove the specific case is valid case or not. Good catch, Sergey!
> 
> However, it should be *fixed* to prevent confusion in future but surely,
> no need to go to the stable. I will send reply to Greg to prevent merging
> it to *stable* when he send review asking to merge.

cool. thanks!

> And next week I will send another fix which *maybe* removes code to get the
> offset in zram_rw_page.

sounds interesting!

-ss

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Sergey Senozhatsky]

On (04/13/17 09:17), Minchan Kim wrote:
> Date: Thu, 13 Apr 2017 09:17:00 +0900
> From: Minchan Kim 
> To: Andrew Morton 
> CC: linux-kernel@vger.kernel.org, Sergey Senozhatsky
>  , kernel-t...@lge.com, Minchan Kim
>  , sta...@vger.kernel.org
> Subject: [PATCH 1/3] zram: fix operator precedence to get offset
> X-Mailer: git-send-email 2.7.4
> 
> In zram_rw_page, the logic to get offset is wrong by operator precedence
> (i.e., "<<" is higher than "&"). With wrong offset, zram can corrupt the
> user's data. This patch fixes it.
> 
> Fixes: 8c7f01025 ("zram: implement rw_page operation of zram")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Minchan Kim 

Reviewed-by: Sergey Senozhatsky 

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Sergey Senozhatsky]

On (04/13/17 09:17), Minchan Kim wrote:
> Date: Thu, 13 Apr 2017 09:17:00 +0900
> From: Minchan Kim 
> To: Andrew Morton 
> CC: linux-kernel@vger.kernel.org, Sergey Senozhatsky
>  , kernel-t...@lge.com, Minchan Kim
>  , sta...@vger.kernel.org
> Subject: [PATCH 1/3] zram: fix operator precedence to get offset
> X-Mailer: git-send-email 2.7.4
> 
> In zram_rw_page, the logic to get offset is wrong by operator precedence
> (i.e., "<<" is higher than "&"). With wrong offset, zram can corrupt the
> user's data. This patch fixes it.
> 
> Fixes: 8c7f01025 ("zram: implement rw_page operation of zram")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Minchan Kim 

Reviewed-by: Sergey Senozhatsky 

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Minchan Kim]

Hi Sergey,

On Fri, Apr 14, 2017 at 02:07:47PM +0900, Sergey Senozhatsky wrote:
> Hello,
> 
> On (04/13/17 09:17), Minchan Kim wrote:
> [..]
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 9e2199060040..83c38a123242 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -930,7 +930,7 @@ static int zram_rw_page(struct block_device *bdev, 
> > sector_t sector,
> > }
> >  
> > index = sector >> SECTORS_PER_PAGE_SHIFT;
> > -   offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> > +   offset = (sector & (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;
> 
> sorry, can it actually produce different results?

I got your point. Actually, offset was wrong but rw_page is called
with PAGE_SIZE io while that offset is related to only partial io
(non-PAGEE size io). IOW, although the wrong offset it is never used
in functions.

To find subtle corruption in ppc64, I added some debug code to
catch up wrong buffer overflow and found it with other bugs but
didn't prove the specific case is valid case or not. Good catch, Sergey!

However, it should be *fixed* to prevent confusion in future but surely,
no need to go to the stable. I will send reply to Greg to prevent merging
it to *stable* when he send review asking to merge.

And next week I will send another fix which *maybe* removes code to get the
offset in zram_rw_page.

Thanks.

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Minchan Kim]

Hi Sergey,

On Fri, Apr 14, 2017 at 02:07:47PM +0900, Sergey Senozhatsky wrote:
> Hello,
> 
> On (04/13/17 09:17), Minchan Kim wrote:
> [..]
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 9e2199060040..83c38a123242 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -930,7 +930,7 @@ static int zram_rw_page(struct block_device *bdev, 
> > sector_t sector,
> > }
> >  
> > index = sector >> SECTORS_PER_PAGE_SHIFT;
> > -   offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> > +   offset = (sector & (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;
> 
> sorry, can it actually produce different results?

I got your point. Actually, offset was wrong but rw_page is called
with PAGE_SIZE io while that offset is related to only partial io
(non-PAGEE size io). IOW, although the wrong offset it is never used
in functions.

To find subtle corruption in ppc64, I added some debug code to
catch up wrong buffer overflow and found it with other bugs but
didn't prove the specific case is valid case or not. Good catch, Sergey!

However, it should be *fixed* to prevent confusion in future but surely,
no need to go to the stable. I will send reply to Greg to prevent merging
it to *stable* when he send review asking to merge.

And next week I will send another fix which *maybe* removes code to get the
offset in zram_rw_page.

Thanks.

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Sergey Senozhatsky]

Hello,

On (04/13/17 09:17), Minchan Kim wrote:
[..]
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 9e2199060040..83c38a123242 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -930,7 +930,7 @@ static int zram_rw_page(struct block_device *bdev, 
> sector_t sector,
>   }
>  
>   index = sector >> SECTORS_PER_PAGE_SHIFT;
> - offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> + offset = (sector & (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;

sorry, can it actually produce different results?

-ss

Re: [PATCH 1/3] zram: fix operator precedence to get offset

[Sergey Senozhatsky]

Hello,

On (04/13/17 09:17), Minchan Kim wrote:
[..]
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 9e2199060040..83c38a123242 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -930,7 +930,7 @@ static int zram_rw_page(struct block_device *bdev, 
> sector_t sector,
>   }
>  
>   index = sector >> SECTORS_PER_PAGE_SHIFT;
> - offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> + offset = (sector & (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;

sorry, can it actually produce different results?

-ss