| Message ID | 20170518104850.14508-3-jack@suse.cz (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote: > There is an off-by-one error in loop termination conditions in > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of > desired range if 'endoff' is page aligned. It doesn't have any visible > effects but still it is good to fix it. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/xfs/xfs_file.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index f371812e20c6..3714b5736fd3 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff( > > index = startoff >> PAGE_SHIFT; > endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount); > - end = endoff >> PAGE_SHIFT; > + end = (endoff - 1) >> PAGE_SHIFT; Hmm.. I think this messes with the want count for the pagevec_lookup(). E.g.: # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file wrote 16384/16384 bytes at offset 0 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec) Whence Result HOLE 12288 Brian > do { > int want; > unsigned nr_pages; > -- > 2.12.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote: > On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote: > > There is an off-by-one error in loop termination conditions in > > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of > > desired range if 'endoff' is page aligned. It doesn't have any visible > > effects but still it is good to fix it. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/xfs/xfs_file.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > > index f371812e20c6..3714b5736fd3 100644 > > --- a/fs/xfs/xfs_file.c > > +++ b/fs/xfs/xfs_file.c > > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff( > > > > index = startoff >> PAGE_SHIFT; > > endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount); > > - end = endoff >> PAGE_SHIFT; > > + end = (endoff - 1) >> PAGE_SHIFT; > > Hmm.. I think this messes with the want count for the pagevec_lookup(). > E.g.: > > # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file > wrote 16384/16384 bytes at offset 0 > 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec) > Whence Result > HOLE 12288 I think the root cause is that the calculation for 'want' is wrong, it has an off-by-one bug too. I sent a patch[1] to fix it, with my patch applied on top of Jan's patchset, your test case passed (report HOLE at 16k). Can you please take a look if it's a correct fix? Thanks! Eryu [1] [PATCH] xfs: fix off-by-one on max nr_pages in xfs_find_get_desired_pgoff() https://www.spinics.net/lists/linux-xfs/msg06980.html > > Brian > > > do { > > int want; > > unsigned nr_pages; > > -- > > 2.12.0 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 23-05-17 11:21:23, Eryu Guan wrote: > On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote: > > On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote: > > > There is an off-by-one error in loop termination conditions in > > > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of > > > desired range if 'endoff' is page aligned. It doesn't have any visible > > > effects but still it is good to fix it. > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > --- > > > fs/xfs/xfs_file.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > > > index f371812e20c6..3714b5736fd3 100644 > > > --- a/fs/xfs/xfs_file.c > > > +++ b/fs/xfs/xfs_file.c > > > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff( > > > > > > index = startoff >> PAGE_SHIFT; > > > endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount); > > > - end = endoff >> PAGE_SHIFT; > > > + end = (endoff - 1) >> PAGE_SHIFT; > > > > Hmm.. I think this messes with the want count for the pagevec_lookup(). > > E.g.: > > > > # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file > > wrote 16384/16384 bytes at offset 0 > > 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec) > > Whence Result > > HOLE 12288 > > I think the root cause is that the calculation for 'want' is wrong, it > has an off-by-one bug too. I sent a patch[1] to fix it, with my patch > applied on top of Jan's patchset, your test case passed (report HOLE at > 16k). Can you please take a look if it's a correct fix? Thanks! Yes, I've messed that up. It is a bug introduced by my series as Brian properly noticed. Thanks guys for noticing and fixing it! Darrick, should I fold in Eryu's fix and send v4 of the series or will you just pick up Eryu's fix? Honza
On Tue, May 23, 2017 at 10:50:44AM +0200, Jan Kara wrote: > On Tue 23-05-17 11:21:23, Eryu Guan wrote: > > On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote: > > > On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote: > > > > There is an off-by-one error in loop termination conditions in > > > > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of > > > > desired range if 'endoff' is page aligned. It doesn't have any visible > > > > effects but still it is good to fix it. > > > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > --- > > > > fs/xfs/xfs_file.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > > > > index f371812e20c6..3714b5736fd3 100644 > > > > --- a/fs/xfs/xfs_file.c > > > > +++ b/fs/xfs/xfs_file.c > > > > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff( > > > > > > > > index = startoff >> PAGE_SHIFT; > > > > endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount); > > > > - end = endoff >> PAGE_SHIFT; > > > > + end = (endoff - 1) >> PAGE_SHIFT; > > > > > > Hmm.. I think this messes with the want count for the pagevec_lookup(). > > > E.g.: > > > > > > # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file > > > wrote 16384/16384 bytes at offset 0 > > > 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec) > > > Whence Result > > > HOLE 12288 > > > > I think the root cause is that the calculation for 'want' is wrong, it > > has an off-by-one bug too. I sent a patch[1] to fix it, with my patch > > applied on top of Jan's patchset, your test case passed (report HOLE at > > 16k). Can you please take a look if it's a correct fix? Thanks! > > Yes, I've messed that up. It is a bug introduced by my series as Brian > properly noticed. Thanks guys for noticing and fixing it! Darrick, should I > fold in Eryu's fix and send v4 of the series or will you just pick up > Eryu's fix? I think it's a separate bug, the issue described in my patch can be reproduced on stock 4.12-rc1 kernel, without your patchset. The situation for ext4 is similar to XFS, it seems not a bug introduced by your patches. Thanks for the review! Eryu > > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 23, 2017 at 07:08:32PM +0800, Eryu Guan wrote: > On Tue, May 23, 2017 at 10:50:44AM +0200, Jan Kara wrote: > > On Tue 23-05-17 11:21:23, Eryu Guan wrote: > > > On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote: > > > > On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote: > > > > > There is an off-by-one error in loop termination conditions in > > > > > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of > > > > > desired range if 'endoff' is page aligned. It doesn't have any visible > > > > > effects but still it is good to fix it. > > > > > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > > --- > > > > > fs/xfs/xfs_file.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > > > > > index f371812e20c6..3714b5736fd3 100644 > > > > > --- a/fs/xfs/xfs_file.c > > > > > +++ b/fs/xfs/xfs_file.c > > > > > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff( > > > > > > > > > > index = startoff >> PAGE_SHIFT; > > > > > endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount); > > > > > - end = endoff >> PAGE_SHIFT; > > > > > + end = (endoff - 1) >> PAGE_SHIFT; > > > > > > > > Hmm.. I think this messes with the want count for the pagevec_lookup(). > > > > E.g.: > > > > > > > > # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file > > > > wrote 16384/16384 bytes at offset 0 > > > > 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec) > > > > Whence Result > > > > HOLE 12288 > > > > > > I think the root cause is that the calculation for 'want' is wrong, it > > > has an off-by-one bug too. I sent a patch[1] to fix it, with my patch > > > applied on top of Jan's patchset, your test case passed (report HOLE at > > > 16k). Can you please take a look if it's a correct fix? Thanks! > > > > Yes, I've messed that up. It is a bug introduced by my series as Brian > > properly noticed. Thanks guys for noticing and fixing it! Darrick, should I > > fold in Eryu's fix and send v4 of the series or will you just pick up > > Eryu's fix? > > I think it's a separate bug, the issue described in my patch can be > reproduced on stock 4.12-rc1 kernel, without your patchset. The > situation for ext4 is similar to XFS, it seems not a bug introduced by > your patches. > > Thanks for the review! > I think there's the possiblity of multiple things going on here. The problem noted above didn't occur without this series applied. Of course, that doesn't mean that there isn't some other problem with the current code that Eryu has reproduced and fixed. In any event, I don't think we should knowingly apply patches with regressions, even if they are fixed up in the same patch series. Could we get this fixed up/combined somehow or another so we at least do not introduce a transient regression (it doesn't matter to me if the independent problem is addressed in a separate patch or at the same time)? Also, Eryu, could you Tested-by this series before it goes in? It seems you have some tests that stress it quite thoroughly. :) Brian > Eryu > > > > > Honza > > -- > > Jan Kara <jack@suse.com> > > SUSE Labs, CR > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 23, 2017 at 08:17:20AM -0400, Brian Foster wrote: > On Tue, May 23, 2017 at 07:08:32PM +0800, Eryu Guan wrote: > > On Tue, May 23, 2017 at 10:50:44AM +0200, Jan Kara wrote: > > > On Tue 23-05-17 11:21:23, Eryu Guan wrote: > > > > On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote: > > > > > On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote: > > > > > > There is an off-by-one error in loop termination conditions in > > > > > > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of > > > > > > desired range if 'endoff' is page aligned. It doesn't have any visible > > > > > > effects but still it is good to fix it. > > > > > > > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > > > --- > > > > > > fs/xfs/xfs_file.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > > > > > > index f371812e20c6..3714b5736fd3 100644 > > > > > > --- a/fs/xfs/xfs_file.c > > > > > > +++ b/fs/xfs/xfs_file.c > > > > > > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff( > > > > > > > > > > > > index = startoff >> PAGE_SHIFT; > > > > > > endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount); > > > > > > - end = endoff >> PAGE_SHIFT; > > > > > > + end = (endoff - 1) >> PAGE_SHIFT; > > > > > > > > > > Hmm.. I think this messes with the want count for the pagevec_lookup(). > > > > > E.g.: > > > > > > > > > > # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file > > > > > wrote 16384/16384 bytes at offset 0 > > > > > 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec) > > > > > Whence Result > > > > > HOLE 12288 > > > > > > > > I think the root cause is that the calculation for 'want' is wrong, it > > > > has an off-by-one bug too. I sent a patch[1] to fix it, with my patch > > > > applied on top of Jan's patchset, your test case passed (report HOLE at > > > > 16k). Can you please take a look if it's a correct fix? Thanks! > > > > > > Yes, I've messed that up. It is a bug introduced by my series as Brian > > > properly noticed. Thanks guys for noticing and fixing it! Darrick, should I > > > fold in Eryu's fix and send v4 of the series or will you just pick up > > > Eryu's fix? > > > > I think it's a separate bug, the issue described in my patch can be > > reproduced on stock 4.12-rc1 kernel, without your patchset. The > > situation for ext4 is similar to XFS, it seems not a bug introduced by > > your patches. > > > > Thanks for the review! > > > > I think there's the possiblity of multiple things going on here. The > problem noted above didn't occur without this series applied. Of course, > that doesn't mean that there isn't some other problem with the current > code that Eryu has reproduced and fixed. > > In any event, I don't think we should knowingly apply patches with > regressions, even if they are fixed up in the same patch series. Could > we get this fixed up/combined somehow or another so we at least do not > introduce a transient regression (it doesn't matter to me if the > independent problem is addressed in a separate patch or at the same > time)? > > Also, Eryu, could you Tested-by this series before it goes in? It seems > you have some tests that stress it quite thoroughly. :) I have no tests more than generic/285 and generic/436, I just run them with different block size xfs/ext4 and on different architectures (x86_64 and ppc64) :) But sure, I'll give a Tested-by tag once we work out which patches should go in. Thanks, Eryu -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 23, 2017 at 10:50:44AM +0200, Jan Kara wrote: > On Tue 23-05-17 11:21:23, Eryu Guan wrote: > > On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote: > > > On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote: > > > > There is an off-by-one error in loop termination conditions in > > > > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of > > > > desired range if 'endoff' is page aligned. It doesn't have any visible > > > > effects but still it is good to fix it. > > > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > --- > > > > fs/xfs/xfs_file.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > > > > index f371812e20c6..3714b5736fd3 100644 > > > > --- a/fs/xfs/xfs_file.c > > > > +++ b/fs/xfs/xfs_file.c > > > > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff( > > > > > > > > index = startoff >> PAGE_SHIFT; > > > > endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount); > > > > - end = endoff >> PAGE_SHIFT; > > > > + end = (endoff - 1) >> PAGE_SHIFT; > > > > > > Hmm.. I think this messes with the want count for the pagevec_lookup(). > > > E.g.: > > > > > > # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file > > > wrote 16384/16384 bytes at offset 0 > > > 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec) > > > Whence Result > > > HOLE 12288 > > > > I think the root cause is that the calculation for 'want' is wrong, it > > has an off-by-one bug too. I sent a patch[1] to fix it, with my patch > > applied on top of Jan's patchset, your test case passed (report HOLE at > > 16k). Can you please take a look if it's a correct fix? Thanks! > > Yes, I've messed that up. It is a bug introduced by my series as Brian > properly noticed. Thanks guys for noticing and fixing it! Darrick, should I > fold in Eryu's fix and send v4 of the series or will you just pick up > Eryu's fix? FWIW it looked like a separate problem to me as well. It appears to me that I could merge Eryu's patch immediately prior to Jan's series; is that ok with everyone? --D > > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 23, 2017 at 08:30:18AM -0700, Darrick J. Wong wrote: > On Tue, May 23, 2017 at 10:50:44AM +0200, Jan Kara wrote: > > On Tue 23-05-17 11:21:23, Eryu Guan wrote: > > > On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote: > > > > On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote: > > > > > There is an off-by-one error in loop termination conditions in > > > > > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of > > > > > desired range if 'endoff' is page aligned. It doesn't have any visible > > > > > effects but still it is good to fix it. > > > > > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > > --- > > > > > fs/xfs/xfs_file.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > > > > > index f371812e20c6..3714b5736fd3 100644 > > > > > --- a/fs/xfs/xfs_file.c > > > > > +++ b/fs/xfs/xfs_file.c > > > > > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff( > > > > > > > > > > index = startoff >> PAGE_SHIFT; > > > > > endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount); > > > > > - end = endoff >> PAGE_SHIFT; > > > > > + end = (endoff - 1) >> PAGE_SHIFT; > > > > > > > > Hmm.. I think this messes with the want count for the pagevec_lookup(). > > > > E.g.: > > > > > > > > # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file > > > > wrote 16384/16384 bytes at offset 0 > > > > 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec) > > > > Whence Result > > > > HOLE 12288 > > > > > > I think the root cause is that the calculation for 'want' is wrong, it > > > has an off-by-one bug too. I sent a patch[1] to fix it, with my patch > > > applied on top of Jan's patchset, your test case passed (report HOLE at > > > 16k). Can you please take a look if it's a correct fix? Thanks! > > > > Yes, I've messed that up. It is a bug introduced by my series as Brian > > properly noticed. Thanks guys for noticing and fixing it! Darrick, should I > > fold in Eryu's fix and send v4 of the series or will you just pick up > > Eryu's fix? > > FWIW it looked like a separate problem to me as well. It appears to me > that I could merge Eryu's patch immediately prior to Jan's series; is > that ok with everyone? > Fine by me if that effectively hides the regression.. Brian > --D > > > > > Honza > > -- > > Jan Kara <jack@suse.com> > > SUSE Labs, CR > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 23, 2017 at 09:05:34PM +0800, Eryu Guan wrote: > On Tue, May 23, 2017 at 08:17:20AM -0400, Brian Foster wrote: > > On Tue, May 23, 2017 at 07:08:32PM +0800, Eryu Guan wrote: > > > On Tue, May 23, 2017 at 10:50:44AM +0200, Jan Kara wrote: > > > > On Tue 23-05-17 11:21:23, Eryu Guan wrote: > > > > > On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote: > > > > > > On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote: > > > > > > > There is an off-by-one error in loop termination conditions in > > > > > > > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of > > > > > > > desired range if 'endoff' is page aligned. It doesn't have any visible > > > > > > > effects but still it is good to fix it. > > > > > > > > > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > > > > --- > > > > > > > fs/xfs/xfs_file.c | 2 +- > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > > > > > > > index f371812e20c6..3714b5736fd3 100644 > > > > > > > --- a/fs/xfs/xfs_file.c > > > > > > > +++ b/fs/xfs/xfs_file.c > > > > > > > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff( > > > > > > > > > > > > > > index = startoff >> PAGE_SHIFT; > > > > > > > endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount); > > > > > > > - end = endoff >> PAGE_SHIFT; > > > > > > > + end = (endoff - 1) >> PAGE_SHIFT; > > > > > > > > > > > > Hmm.. I think this messes with the want count for the pagevec_lookup(). > > > > > > E.g.: > > > > > > > > > > > > # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file > > > > > > wrote 16384/16384 bytes at offset 0 > > > > > > 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec) > > > > > > Whence Result > > > > > > HOLE 12288 > > > > > > > > > > I think the root cause is that the calculation for 'want' is wrong, it > > > > > has an off-by-one bug too. I sent a patch[1] to fix it, with my patch > > > > > applied on top of Jan's patchset, your test case passed (report HOLE at > > > > > 16k). Can you please take a look if it's a correct fix? Thanks! > > > > > > > > Yes, I've messed that up. It is a bug introduced by my series as Brian > > > > properly noticed. Thanks guys for noticing and fixing it! Darrick, should I > > > > fold in Eryu's fix and send v4 of the series or will you just pick up > > > > Eryu's fix? > > > > > > I think it's a separate bug, the issue described in my patch can be > > > reproduced on stock 4.12-rc1 kernel, without your patchset. The > > > situation for ext4 is similar to XFS, it seems not a bug introduced by > > > your patches. > > > > > > Thanks for the review! > > > > > > > I think there's the possiblity of multiple things going on here. The > > problem noted above didn't occur without this series applied. Of course, > > that doesn't mean that there isn't some other problem with the current > > code that Eryu has reproduced and fixed. > > > > In any event, I don't think we should knowingly apply patches with > > regressions, even if they are fixed up in the same patch series. Could > > we get this fixed up/combined somehow or another so we at least do not > > introduce a transient regression (it doesn't matter to me if the > > independent problem is addressed in a separate patch or at the same > > time)? > > > > Also, Eryu, could you Tested-by this series before it goes in? It seems > > you have some tests that stress it quite thoroughly. :) > > I have no tests more than generic/285 and generic/436, I just run them > with different block size xfs/ext4 and on different architectures > (x86_64 and ppc64) :) But sure, I'll give a Tested-by tag once we work > out which patches should go in. FWIW I added Eryu's patch and then all of Jan's to my 4.12-fixes branch: https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=xfs-4.12-fixes (Have a look to make sure I didn't miss anything, but be warned that I have no reservations against reworking a personal repo branch arbitrarily). Then I ran all the tests I saw mentioned (g/285, g/436, 4k and 1k block sizes) and didn't see any problems, so I'm now running all the other tests. If nothing blows up then that'll end up as the next push to Linus around the end of the week. --D > > Thanks, > Eryu > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index f371812e20c6..3714b5736fd3 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff( index = startoff >> PAGE_SHIFT; endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount); - end = endoff >> PAGE_SHIFT; + end = (endoff - 1) >> PAGE_SHIFT; do { int want; unsigned nr_pages;
There is an off-by-one error in loop termination conditions in xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of desired range if 'endoff' is page aligned. It doesn't have any visible effects but still it is good to fix it. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/xfs/xfs_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)