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@xxxxxxxxxxxxxxx # v3.7+ > > > Signed-off-by: Eryu Guan <eguan@xxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxx > > > 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@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
