[PATCH] clean_bdev_aliases: Prevent cleaning blocks that are not in block range

[PATCH] clean_bdev_aliases: Prevent cleaning blocks that are not in block range

[Chandan Rajendra]

The first block to be cleaned may start at a non-zero page offset. In
such a scenario clean_bdev_aliases() will end up cleaning blocks that
do not fall in the range of blocks to be cleaned. This commit fixes the
issue by skipping blocks that do not fall in valid block range.

Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
---
 fs/buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 1df2bd5..28484b3 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1660,7 +1660,7 @@ void clean_bdev_aliases(struct block_device *bdev, sector_t block, sector_t len)
 			head = page_buffers(page);
 			bh = head;
 			do {
-				if (!buffer_mapped(bh))
+				if (!buffer_mapped(bh) || (bh->b_blocknr < block))
 					goto next;
 				if (bh->b_blocknr >= block + len)
 					break;
-- 
2.5.5

Re: [PATCH] clean_bdev_aliases: Prevent cleaning blocks that are not in block range

[Jan Kara]

On Sun 25-12-16 19:01:03, Chandan Rajendra wrote:
> The first block to be cleaned may start at a non-zero page offset. In
> such a scenario clean_bdev_aliases() will end up cleaning blocks that
> do not fall in the range of blocks to be cleaned. This commit fixes the
> issue by skipping blocks that do not fall in valid block range.
> 
> Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>

Ah, very good catch! How did you spot this?

The patch looks good. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

Jens, please merge this fix quickly as we may end up discarding changes to
innocent metadata blocks due to this... Thanks!

								Honza
> ---
>  fs/buffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 1df2bd5..28484b3 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1660,7 +1660,7 @@ void clean_bdev_aliases(struct block_device *bdev, sector_t block, sector_t len)
>  			head = page_buffers(page);
>  			bh = head;
>  			do {
> -				if (!buffer_mapped(bh))
> +				if (!buffer_mapped(bh) || (bh->b_blocknr < block))
>  					goto next;
>  				if (bh->b_blocknr >= block + len)
>  					break;
> -- 
> 2.5.5
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] clean_bdev_aliases: Prevent cleaning blocks that are not in block range

[Eryu Guan]

On Mon, Jan 02, 2017 at 04:58:03PM +0100, Jan Kara wrote:
> On Sun 25-12-16 19:01:03, Chandan Rajendra wrote:
> > The first block to be cleaned may start at a non-zero page offset. In
> > such a scenario clean_bdev_aliases() will end up cleaning blocks that
> > do not fall in the range of blocks to be cleaned. This commit fixes the
> > issue by skipping blocks that do not fall in valid block range.
> > 
> > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> 
> Ah, very good catch! How did you spot this?

I failed to notice this patch, and I came up with a same patch today
myself, and I'm still testing it.

I found this by xfstests, many tests (tens of tests) failed fsck after
test when testing extN if blocksize < pagesize. E.g. generic/013 could
reproduce the fs corruption quite reliablely for me.

Reviewed-by: Eryu Guan <eguan@redhat.com>

> 
> The patch looks good. You can add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> Jens, please merge this fix quickly as we may end up discarding changes to
> innocent metadata blocks due to this... Thanks!
> 
> 								Honza
> > ---
> >  fs/buffer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index 1df2bd5..28484b3 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -1660,7 +1660,7 @@ void clean_bdev_aliases(struct block_device *bdev, sector_t block, sector_t len)
> >  			head = page_buffers(page);
> >  			bh = head;
> >  			do {
> > -				if (!buffer_mapped(bh))
> > +				if (!buffer_mapped(bh) || (bh->b_blocknr < block))
> >  					goto next;
> >  				if (bh->b_blocknr >= block + len)
> >  					break;
> > -- 
> > 2.5.5
> > 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clean_bdev_aliases: Prevent cleaning blocks that are not in block range

[Jens Axboe]

On 12/25/2016 06:31 AM, Chandan Rajendra wrote:
> The first block to be cleaned may start at a non-zero page offset. In
> such a scenario clean_bdev_aliases() will end up cleaning blocks that
> do not fall in the range of blocks to be cleaned. This commit fixes the
> issue by skipping blocks that do not fall in valid block range.

Queued up, thanks.

-- 
Jens Axboe

Re: [PATCH] clean_bdev_aliases: Prevent cleaning blocks that are not in block range

[Jan Kara]

On Tue 03-01-17 00:21:45, Eryu Guan wrote:
> On Mon, Jan 02, 2017 at 04:58:03PM +0100, Jan Kara wrote:
> > On Sun 25-12-16 19:01:03, Chandan Rajendra wrote:
> > > The first block to be cleaned may start at a non-zero page offset. In
> > > such a scenario clean_bdev_aliases() will end up cleaning blocks that
> > > do not fall in the range of blocks to be cleaned. This commit fixes the
> > > issue by skipping blocks that do not fall in valid block range.
> > > 
> > > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> > 
> > Ah, very good catch! How did you spot this?
> 
> I failed to notice this patch, and I came up with a same patch today
> myself, and I'm still testing it.
> 
> I found this by xfstests, many tests (tens of tests) failed fsck after
> test when testing extN if blocksize < pagesize. E.g. generic/013 could
> reproduce the fs corruption quite reliablely for me.
> 
> Reviewed-by: Eryu Guan <eguan@redhat.com>

Thanks! Yeah, I had run xfstests with those patches but not with blocksize
< pagesize :-| Shit happens... need to be more careful next time.

								Honza

> > The patch looks good. You can add:
> > 
> > Reviewed-by: Jan Kara <jack@suse.cz>
> > 
> > Jens, please merge this fix quickly as we may end up discarding changes to
> > innocent metadata blocks due to this... Thanks!
> > 
> > 								Honza
> > > ---
> > >  fs/buffer.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/buffer.c b/fs/buffer.c
> > > index 1df2bd5..28484b3 100644
> > > --- a/fs/buffer.c
> > > +++ b/fs/buffer.c
> > > @@ -1660,7 +1660,7 @@ void clean_bdev_aliases(struct block_device *bdev, sector_t block, sector_t len)
> > >  			head = page_buffers(page);
> > >  			bh = head;
> > >  			do {
> > > -				if (!buffer_mapped(bh))
> > > +				if (!buffer_mapped(bh) || (bh->b_blocknr < block))
> > >  					goto next;
> > >  				if (bh->b_blocknr >= block + len)
> > >  					break;
> > > -- 
> > > 2.5.5
> > > 
> > -- 
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-block" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR