* Re: [PATCH] xfs: fix off-by-one on max nr_pages in xfs_find_get_desired_pgoff()
2017-05-21 7:59 [PATCH] xfs: fix off-by-one on max nr_pages in xfs_find_get_desired_pgoff() Eryu Guan
@ 2017-05-23 8:36 ` Jan Kara
2017-05-23 12:35 ` Brian Foster
2017-05-23 15:33 ` Darrick J. Wong
2 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2017-05-23 8:36 UTC (permalink / raw)
To: Eryu Guan; +Cc: linux-xfs
On Sun 21-05-17 15:59:39, Eryu Guan wrote:
> xfs_find_get_desired_pgoff() is used to search for offset of hole or
> data in page range [index, end] (both inclusive), and the max number
> of pages to search should be at least one, if end == index.
> Otherwise the only page is missed and no hole or data is found,
> which is not correct.
>
> When block size is smaller than page size, this can be demonstrated
> by preallocating a file with size smaller than page size and writing
> data to the last block. E.g. run this xfs_io command on a 1k block
> size XFS on x86_64 host.
>
> # xfs_io -fc "falloc 0 3k" -c "pwrite 2k 1k" \
> -c "seek -d 0" /mnt/xfs/testfile
> wrote 1024/1024 bytes at offset 2048
> 1 KiB, 1 ops; 0.0000 sec (33.675 MiB/sec and 34482.7586 ops/sec)
> Whence Result
> DATA EOF
>
> Data at offset 2k was missed, and lseek(2) returned ENXIO.
>
> This is unconvered by generic/285 subtest 07 and 08 on ppc64 host,
> where pagesize is 64k. Because a recent change to generic/285
> reduced the preallocated file size to smaller than 64k.
>
> Cc: stable@vger.kernel.org # v3.7+
> Signed-off-by: Eryu Guan <eguan@redhat.com>
Looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
BTW, this was actually introduced by my recent SEEK_HOLE fixes so Darrick,
please put this in one batch with them. Thanks!
Honza
> ---
> 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 35703a8..aefa213 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1049,7 +1049,7 @@ xfs_find_get_desired_pgoff(
> unsigned nr_pages;
> unsigned int i;
>
> - want = min_t(pgoff_t, end - index, PAGEVEC_SIZE);
> + want = min_t(pgoff_t, end - index, PAGEVEC_SIZE - 1) + 1;
> nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index,
> want);
> /*
> --
> 2.9.4
>
> --
> 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
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: fix off-by-one on max nr_pages in xfs_find_get_desired_pgoff()
2017-05-21 7:59 [PATCH] xfs: fix off-by-one on max nr_pages in xfs_find_get_desired_pgoff() Eryu Guan
2017-05-23 8:36 ` Jan Kara
@ 2017-05-23 12:35 ` Brian Foster
2017-05-23 13:00 ` Eryu Guan
2017-05-23 15:33 ` Darrick J. Wong
2 siblings, 1 reply; 6+ messages in thread
From: Brian Foster @ 2017-05-23 12:35 UTC (permalink / raw)
To: Eryu Guan; +Cc: linux-xfs
On Sun, May 21, 2017 at 03:59:39PM +0800, Eryu Guan wrote:
> xfs_find_get_desired_pgoff() is used to search for offset of hole or
> data in page range [index, end] (both inclusive), and the max number
> of pages to search should be at least one, if end == index.
> Otherwise the only page is missed and no hole or data is found,
> which is not correct.
>
> When block size is smaller than page size, this can be demonstrated
> by preallocating a file with size smaller than page size and writing
> data to the last block. E.g. run this xfs_io command on a 1k block
> size XFS on x86_64 host.
>
> # xfs_io -fc "falloc 0 3k" -c "pwrite 2k 1k" \
> -c "seek -d 0" /mnt/xfs/testfile
> wrote 1024/1024 bytes at offset 2048
> 1 KiB, 1 ops; 0.0000 sec (33.675 MiB/sec and 34482.7586 ops/sec)
> Whence Result
> DATA EOF
>
Ok, so this does look like a different problem.
> Data at offset 2k was missed, and lseek(2) returned ENXIO.
>
> This is unconvered by generic/285 subtest 07 and 08 on ppc64 host,
> where pagesize is 64k. Because a recent change to generic/285
> reduced the preallocated file size to smaller than 64k.
>
> Cc: stable@vger.kernel.org # v3.7+
> Signed-off-by: Eryu Guan <eguan@redhat.com>
Could we fold this patch into Jan's patch 2 or vice versa (retaining
Eryu's commit log and credit)?
> ---
> 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 35703a8..aefa213 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1049,7 +1049,7 @@ xfs_find_get_desired_pgoff(
> unsigned nr_pages;
> unsigned int i;
>
> - want = min_t(pgoff_t, end - index, PAGEVEC_SIZE);
> + want = min_t(pgoff_t, end - index, PAGEVEC_SIZE - 1) + 1;
Kind of a nit, but could we do the following?
want = min_t(pgoff_t, end - index + 1, PAGEVEC_SIZE);
It seems more clear to me given that end would be inclusive after Jan's
patch.
Brian
> nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index,
> want);
> /*
> --
> 2.9.4
>
> --
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: fix off-by-one on max nr_pages in xfs_find_get_desired_pgoff()
2017-05-23 12:35 ` Brian Foster
@ 2017-05-23 13:00 ` Eryu Guan
2017-05-23 13:27 ` Brian Foster
0 siblings, 1 reply; 6+ messages in thread
From: Eryu Guan @ 2017-05-23 13:00 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Tue, May 23, 2017 at 08:35:39AM -0400, Brian Foster wrote:
> On Sun, May 21, 2017 at 03:59:39PM +0800, Eryu Guan wrote:
> > xfs_find_get_desired_pgoff() is used to search for offset of hole or
> > data in page range [index, end] (both inclusive), and the max number
> > of pages to search should be at least one, if end == index.
> > Otherwise the only page is missed and no hole or data is found,
> > which is not correct.
> >
> > When block size is smaller than page size, this can be demonstrated
> > by preallocating a file with size smaller than page size and writing
> > data to the last block. E.g. run this xfs_io command on a 1k block
> > size XFS on x86_64 host.
> >
> > # xfs_io -fc "falloc 0 3k" -c "pwrite 2k 1k" \
> > -c "seek -d 0" /mnt/xfs/testfile
> > wrote 1024/1024 bytes at offset 2048
> > 1 KiB, 1 ops; 0.0000 sec (33.675 MiB/sec and 34482.7586 ops/sec)
> > Whence Result
> > DATA EOF
> >
>
> Ok, so this does look like a different problem.
>
> > Data at offset 2k was missed, and lseek(2) returned ENXIO.
> >
> > This is unconvered by generic/285 subtest 07 and 08 on ppc64 host,
> > where pagesize is 64k. Because a recent change to generic/285
> > reduced the preallocated file size to smaller than 64k.
> >
> > Cc: stable@vger.kernel.org # v3.7+
> > Signed-off-by: Eryu Guan <eguan@redhat.com>
>
> Could we fold this patch into Jan's patch 2 or vice versa (retaining
> Eryu's commit log and credit)?
>
> > ---
> > 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 35703a8..aefa213 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -1049,7 +1049,7 @@ xfs_find_get_desired_pgoff(
> > unsigned nr_pages;
> > unsigned int i;
> >
> > - want = min_t(pgoff_t, end - index, PAGEVEC_SIZE);
> > + want = min_t(pgoff_t, end - index, PAGEVEC_SIZE - 1) + 1;
>
> Kind of a nit, but could we do the following?
>
> want = min_t(pgoff_t, end - index + 1, PAGEVEC_SIZE);
>
> It seems more clear to me given that end would be inclusive after Jan's
> patch.
I thought about it too, but I searched for other places doing the same
calculation and they are all comparing (end-index) with (PAGEVEC_SIZE-1)
then plus one, e.g. mm/truncate.c::invalidate_inode_pages2_range(). I
guess it's meant to prevent (end - index + 1) from overflowing, i.e.
(end(ULONG_MAX) - index(0) + 1).
Thanks for the review!
Eryu
>
> Brian
>
> > nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index,
> > want);
> > /*
> > --
> > 2.9.4
> >
> > --
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: fix off-by-one on max nr_pages in xfs_find_get_desired_pgoff()
2017-05-23 13:00 ` Eryu Guan
@ 2017-05-23 13:27 ` Brian Foster
0 siblings, 0 replies; 6+ messages in thread
From: Brian Foster @ 2017-05-23 13:27 UTC (permalink / raw)
To: Eryu Guan; +Cc: linux-xfs
On Tue, May 23, 2017 at 09:00:28PM +0800, Eryu Guan wrote:
> On Tue, May 23, 2017 at 08:35:39AM -0400, Brian Foster wrote:
> > On Sun, May 21, 2017 at 03:59:39PM +0800, Eryu Guan wrote:
> > > xfs_find_get_desired_pgoff() is used to search for offset of hole or
> > > data in page range [index, end] (both inclusive), and the max number
> > > of pages to search should be at least one, if end == index.
> > > Otherwise the only page is missed and no hole or data is found,
> > > which is not correct.
> > >
> > > When block size is smaller than page size, this can be demonstrated
> > > by preallocating a file with size smaller than page size and writing
> > > data to the last block. E.g. run this xfs_io command on a 1k block
> > > size XFS on x86_64 host.
> > >
> > > # xfs_io -fc "falloc 0 3k" -c "pwrite 2k 1k" \
> > > -c "seek -d 0" /mnt/xfs/testfile
> > > wrote 1024/1024 bytes at offset 2048
> > > 1 KiB, 1 ops; 0.0000 sec (33.675 MiB/sec and 34482.7586 ops/sec)
> > > Whence Result
> > > DATA EOF
> > >
> >
> > Ok, so this does look like a different problem.
> >
> > > Data at offset 2k was missed, and lseek(2) returned ENXIO.
> > >
> > > This is unconvered by generic/285 subtest 07 and 08 on ppc64 host,
> > > where pagesize is 64k. Because a recent change to generic/285
> > > reduced the preallocated file size to smaller than 64k.
> > >
> > > Cc: stable@vger.kernel.org # v3.7+
> > > Signed-off-by: Eryu Guan <eguan@redhat.com>
> >
> > Could we fold this patch into Jan's patch 2 or vice versa (retaining
> > Eryu's commit log and credit)?
> >
> > > ---
> > > 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 35703a8..aefa213 100644
> > > --- a/fs/xfs/xfs_file.c
> > > +++ b/fs/xfs/xfs_file.c
> > > @@ -1049,7 +1049,7 @@ xfs_find_get_desired_pgoff(
> > > unsigned nr_pages;
> > > unsigned int i;
> > >
> > > - want = min_t(pgoff_t, end - index, PAGEVEC_SIZE);
> > > + want = min_t(pgoff_t, end - index, PAGEVEC_SIZE - 1) + 1;
> >
> > Kind of a nit, but could we do the following?
> >
> > want = min_t(pgoff_t, end - index + 1, PAGEVEC_SIZE);
> >
> > It seems more clear to me given that end would be inclusive after Jan's
> > patch.
>
> I thought about it too, but I searched for other places doing the same
> calculation and they are all comparing (end-index) with (PAGEVEC_SIZE-1)
> then plus one, e.g. mm/truncate.c::invalidate_inode_pages2_range(). I
> guess it's meant to prevent (end - index + 1) from overflowing, i.e.
> (end(ULONG_MAX) - index(0) + 1).
>
Ah, I see. Disregard this comment then, thanks!
Brian
> Thanks for the review!
>
> Eryu
>
> >
> > Brian
> >
> > > nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index,
> > > want);
> > > /*
> > > --
> > > 2.9.4
> > >
> > > --
> > > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: fix off-by-one on max nr_pages in xfs_find_get_desired_pgoff()
2017-05-21 7:59 [PATCH] xfs: fix off-by-one on max nr_pages in xfs_find_get_desired_pgoff() Eryu Guan
2017-05-23 8:36 ` Jan Kara
2017-05-23 12:35 ` Brian Foster
@ 2017-05-23 15:33 ` Darrick J. Wong
2 siblings, 0 replies; 6+ messages in thread
From: Darrick J. Wong @ 2017-05-23 15:33 UTC (permalink / raw)
To: Eryu Guan; +Cc: linux-xfs
On Sun, May 21, 2017 at 03:59:39PM +0800, Eryu Guan wrote:
> xfs_find_get_desired_pgoff() is used to search for offset of hole or
> data in page range [index, end] (both inclusive), and the max number
> of pages to search should be at least one, if end == index.
> Otherwise the only page is missed and no hole or data is found,
> which is not correct.
>
> When block size is smaller than page size, this can be demonstrated
> by preallocating a file with size smaller than page size and writing
> data to the last block. E.g. run this xfs_io command on a 1k block
> size XFS on x86_64 host.
>
> # xfs_io -fc "falloc 0 3k" -c "pwrite 2k 1k" \
> -c "seek -d 0" /mnt/xfs/testfile
> wrote 1024/1024 bytes at offset 2048
> 1 KiB, 1 ops; 0.0000 sec (33.675 MiB/sec and 34482.7586 ops/sec)
> Whence Result
> DATA EOF
>
> Data at offset 2k was missed, and lseek(2) returned ENXIO.
>
> This is unconvered by generic/285 subtest 07 and 08 on ppc64 host,
"uncovered", will fix it when I commit and push to testbox.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> where pagesize is 64k. Because a recent change to generic/285
> reduced the preallocated file size to smaller than 64k.
>
> Cc: stable@vger.kernel.org # v3.7+
> Signed-off-by: Eryu Guan <eguan@redhat.com>
> ---
> 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 35703a8..aefa213 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1049,7 +1049,7 @@ xfs_find_get_desired_pgoff(
> unsigned nr_pages;
> unsigned int i;
>
> - want = min_t(pgoff_t, end - index, PAGEVEC_SIZE);
> + want = min_t(pgoff_t, end - index, PAGEVEC_SIZE - 1) + 1;
> nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index,
> want);
> /*
> --
> 2.9.4
>
> --
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread