[PATCH v2] xfs: Make fiemap works with sparse file.

[PATCH v2] xfs: Make fiemap works with sparse file.

[Tao Ma]

In xfs_vn_fiemap, we set bvm_count to fi_extent_max + 1 and want
to return fi_extent_max extents, but actually it won't work for
a sparse file. The reason is that in xfs_getbmap we will
calculate holes and set it in 'out', while out is malloced by
bmv_count(fi_extent_max+1) which didn't consider holes. So in the
worst case, if 'out' vector looks like
[hole, extent, hole, extent, hole, ... hole, extent, hole],
we will only return half of fi_extent_max extents.

So in xfs_vn_fiemap, we should consider this worst case. If the
user wants fi_extent_max extents, we need a 'out' with size of
2 *fi_extent_max + 2(one more the header).

Cc: Alex Elder <aelder@sgi.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Dave Chinner <david@fromorbit.com>
Acked-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/xfs/linux-2.6/xfs_iops.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_iops.c b/fs/xfs/linux-2.6/xfs_iops.c
index 9c8019c..1db92e3 100644
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -672,9 +672,21 @@ xfs_vn_fiemap(
 	else
 		bm.bmv_length = BTOBB(length);
 
-	/* We add one because in getbmap world count includes the header */
+	/*
+	 * It is a bit tricky for us to calculate the bmv_count from
+	 * fi_extent_max.
+	 * If we support to return fi_extent_max extents to the user,
+	 * we need at most 2 * fi_extent_max + 1 for bmv_count since
+	 * in xfs_getbmap we will calculate holes while fi_extent_max
+	 * don't have them. So in the worst case, bmv can looks like
+	 * [hole, extent, hole, extent, hole, ... hole, extent, hole].
+	 * So there will be 2 *fi_extent_max + 1.
+	 * What's more, in getbmap world count have to include the
+	 * header, so we need another bmv. So the total number will
+	 * be 2 * fieinfo->fi_extents_max + 2.
+	 */
 	bm.bmv_count = !fieinfo->fi_extents_max ? MAXEXTNUM :
-					fieinfo->fi_extents_max + 1;
+			2 * fieinfo->fi_extents_max + 2;
 	bm.bmv_count = min_t(__s32, bm.bmv_count,
 			     (PAGE_SIZE * 16 / sizeof(struct getbmapx)));
 	bm.bmv_iflags = BMV_IF_PREALLOC;
-- 
1.5.5

[PATCH v2] xfs: Make fiemap works with sparse file.

[Tao Ma]

In xfs_vn_fiemap, we set bvm_count to fi_extent_max + 1 and want
to return fi_extent_max extents, but actually it won't work for
a sparse file. The reason is that in xfs_getbmap we will
calculate holes and set it in 'out', while out is malloced by
bmv_count(fi_extent_max+1) which didn't consider holes. So in the
worst case, if 'out' vector looks like
[hole, extent, hole, extent, hole, ... hole, extent, hole],
we will only return half of fi_extent_max extents.

So in xfs_vn_fiemap, we should consider this worst case. If the
user wants fi_extent_max extents, we need a 'out' with size of
2 *fi_extent_max + 2(one more the header).

Cc: Alex Elder <aelder@sgi.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Dave Chinner <david@fromorbit.com>
Acked-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/xfs/linux-2.6/xfs_iops.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_iops.c b/fs/xfs/linux-2.6/xfs_iops.c
index 9c8019c..1db92e3 100644
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -672,9 +672,21 @@ xfs_vn_fiemap(
 	else
 		bm.bmv_length = BTOBB(length);
 
-	/* We add one because in getbmap world count includes the header */
+	/*
+	 * It is a bit tricky for us to calculate the bmv_count from
+	 * fi_extent_max.
+	 * If we support to return fi_extent_max extents to the user,
+	 * we need at most 2 * fi_extent_max + 1 for bmv_count since
+	 * in xfs_getbmap we will calculate holes while fi_extent_max
+	 * don't have them. So in the worst case, bmv can looks like
+	 * [hole, extent, hole, extent, hole, ... hole, extent, hole].
+	 * So there will be 2 *fi_extent_max + 1.
+	 * What's more, in getbmap world count have to include the
+	 * header, so we need another bmv. So the total number will
+	 * be 2 * fieinfo->fi_extents_max + 2.
+	 */
 	bm.bmv_count = !fieinfo->fi_extents_max ? MAXEXTNUM :
-					fieinfo->fi_extents_max + 1;
+			2 * fieinfo->fi_extents_max + 2;
 	bm.bmv_count = min_t(__s32, bm.bmv_count,
 			     (PAGE_SIZE * 16 / sizeof(struct getbmapx)));
 	bm.bmv_iflags = BMV_IF_PREALLOC;
-- 
1.5.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Dave Chinner]

On Sat, Jun 12, 2010 at 10:08:15AM +0800, Tao Ma wrote:
> In xfs_vn_fiemap, we set bvm_count to fi_extent_max + 1 and want
> to return fi_extent_max extents, but actually it won't work for
> a sparse file.

Define "won't work". i.e. what's the test case?  I just created a
sparse file and checked it, and it reported all the extents in it:

# xfs_bmap -vp testfile 
testfile:
 EXT: FILE-OFFSET      BLOCK-RANGE      AG AG-OFFSET        TOTAL FLAGS
   0: [0..7]:          hole                                     8
   1: [8..15]:         96..103           0 (96..103)            8 00000
   2: [16..23]:        hole                                     8
   3: [24..31]:        112..119          0 (112..119)           8 00000
   4: [32..39]:        hole                                     8
   5: [40..47]:        128..135          0 (128..135)           8 00000
   6: [48..55]:        hole                                     8
   7: [56..63]:        144..151          0 (144..151)           8 00000
   8: [64..71]:        hole                                     8
   9: [72..79]:        160..167          0 (160..167)           8 00000
  10: [80..87]:        hole                                     8
  11: [88..95]:        176..183          0 (176..183)           8 00000
  12: [96..103]:       hole                                     8
  13: [104..111]:      192..199          0 (192..199)           8 00000
  14: [112..119]:      hole                                     8
  15: [120..127]:      208..215          0 (208..215)           8 00000
# filefrag -v testfile 
Filesystem type is: 58465342
File size of testfile is 65536 (16 blocks, blocksize 4096)
 ext logical physical expected length flags
   0       1       12               1 
   1       3       14       12      1 
   2       5       16       14      1 
   3       7       18       16      1 
   4       9       20       18      1 
   5      11       22       20      1 
   6      13       24       22      1 
   7      15       26       24      1 eof
testfile: 9 extents found
#

FWIW, filefrag seems busted - the file has 8 extents, not 9.

For a more fragmented sparse file (25,000 extents):

# for i in `seq 1 2 50000`; do dd if=/dev/zero of=testfile bs=4k count=1 seek=$i; done
....
# xfs_bmap -vp testfile | grep -v hole | wc -l
25002
# filefrag -v testfile |tail -1
testfile: 25001 extents found

So taking away the 2 header lines from xfs_bmap output we have 25000
extents, and filefrag has over-counted by one again. However, we are
we are definitely finding all the extents through fiemap...

> The reason is that in xfs_getbmap we will
> calculate holes and set it in 'out', while out is malloced by
> bmv_count(fi_extent_max+1) which didn't consider holes. So in the
> worst case, if 'out' vector looks like
> [hole, extent, hole, extent, hole, ... hole, extent, hole],
> we will only return half of fi_extent_max extents.

Right, it's not broken, we simply return less than fi_extent_mex
extents when there are holes. I don't see that as a problem as
applications have to handle that case anyway, and....

> So in xfs_vn_fiemap, we should consider this worst case. If the
> user wants fi_extent_max extents, we need a 'out' with size of
> 2 *fi_extent_max + 2(one more the header).

That's rather dangerous, I think. It relies on other code to catch
the buffer overrun that this sets up for fragmented, non-sparse
files. Personally I'd much prefer to return fewer extents for sparse
files than to add a landmine like this into the kernel code....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Dave Chinner]

On Sat, Jun 12, 2010 at 10:08:15AM +0800, Tao Ma wrote:
> In xfs_vn_fiemap, we set bvm_count to fi_extent_max + 1 and want
> to return fi_extent_max extents, but actually it won't work for
> a sparse file.

Define "won't work". i.e. what's the test case?  I just created a
sparse file and checked it, and it reported all the extents in it:

# xfs_bmap -vp testfile 
testfile:
 EXT: FILE-OFFSET      BLOCK-RANGE      AG AG-OFFSET        TOTAL FLAGS
   0: [0..7]:          hole                                     8
   1: [8..15]:         96..103           0 (96..103)            8 00000
   2: [16..23]:        hole                                     8
   3: [24..31]:        112..119          0 (112..119)           8 00000
   4: [32..39]:        hole                                     8
   5: [40..47]:        128..135          0 (128..135)           8 00000
   6: [48..55]:        hole                                     8
   7: [56..63]:        144..151          0 (144..151)           8 00000
   8: [64..71]:        hole                                     8
   9: [72..79]:        160..167          0 (160..167)           8 00000
  10: [80..87]:        hole                                     8
  11: [88..95]:        176..183          0 (176..183)           8 00000
  12: [96..103]:       hole                                     8
  13: [104..111]:      192..199          0 (192..199)           8 00000
  14: [112..119]:      hole                                     8
  15: [120..127]:      208..215          0 (208..215)           8 00000
# filefrag -v testfile 
Filesystem type is: 58465342
File size of testfile is 65536 (16 blocks, blocksize 4096)
 ext logical physical expected length flags
   0       1       12               1 
   1       3       14       12      1 
   2       5       16       14      1 
   3       7       18       16      1 
   4       9       20       18      1 
   5      11       22       20      1 
   6      13       24       22      1 
   7      15       26       24      1 eof
testfile: 9 extents found
#

FWIW, filefrag seems busted - the file has 8 extents, not 9.

For a more fragmented sparse file (25,000 extents):

# for i in `seq 1 2 50000`; do dd if=/dev/zero of=testfile bs=4k count=1 seek=$i; done
....
# xfs_bmap -vp testfile | grep -v hole | wc -l
25002
# filefrag -v testfile |tail -1
testfile: 25001 extents found

So taking away the 2 header lines from xfs_bmap output we have 25000
extents, and filefrag has over-counted by one again. However, we are
we are definitely finding all the extents through fiemap...

> The reason is that in xfs_getbmap we will
> calculate holes and set it in 'out', while out is malloced by
> bmv_count(fi_extent_max+1) which didn't consider holes. So in the
> worst case, if 'out' vector looks like
> [hole, extent, hole, extent, hole, ... hole, extent, hole],
> we will only return half of fi_extent_max extents.

Right, it's not broken, we simply return less than fi_extent_mex
extents when there are holes. I don't see that as a problem as
applications have to handle that case anyway, and....

> So in xfs_vn_fiemap, we should consider this worst case. If the
> user wants fi_extent_max extents, we need a 'out' with size of
> 2 *fi_extent_max + 2(one more the header).

That's rather dangerous, I think. It relies on other code to catch
the buffer overrun that this sets up for fragmented, non-sparse
files. Personally I'd much prefer to return fewer extents for sparse
files than to add a landmine like this into the kernel code....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Tao Ma]

On 06/14/2010 08:27 AM, Dave Chinner wrote:
> On Sat, Jun 12, 2010 at 10:08:15AM +0800, Tao Ma wrote:
>> In xfs_vn_fiemap, we set bvm_count to fi_extent_max + 1 and want
>> to return fi_extent_max extents, but actually it won't work for
>> a sparse file.
>
> Define "won't work". i.e. what's the test case?  I just created a
> sparse file and checked it, and it reported all the extents in it:
>
> # xfs_bmap -vp testfile
> testfile:
>   EXT: FILE-OFFSET      BLOCK-RANGE      AG AG-OFFSET        TOTAL FLAGS
>     0: [0..7]:          hole                                     8
>     1: [8..15]:         96..103           0 (96..103)            8 00000
>     2: [16..23]:        hole                                     8
>     3: [24..31]:        112..119          0 (112..119)           8 00000
>     4: [32..39]:        hole                                     8
>     5: [40..47]:        128..135          0 (128..135)           8 00000
>     6: [48..55]:        hole                                     8
>     7: [56..63]:        144..151          0 (144..151)           8 00000
>     8: [64..71]:        hole                                     8
>     9: [72..79]:        160..167          0 (160..167)           8 00000
>    10: [80..87]:        hole                                     8
>    11: [88..95]:        176..183          0 (176..183)           8 00000
>    12: [96..103]:       hole                                     8
>    13: [104..111]:      192..199          0 (192..199)           8 00000
>    14: [112..119]:      hole                                     8
>    15: [120..127]:      208..215          0 (208..215)           8 00000
ok, so let me explain it. In commit 
2d1ff3c75a4642062d314634290be6d8da4ffb03, I add the mode for extent 
query of fiemap for xfs. So with your test file, it will return that we 
have 8 extents(because in xfs_fiemap_format we don't return holes). So 
normally and naturally, a user begin to iterate all the extents by doing

fiemap = malloc(sizeof(fiemap) + 8 * sizeof(struct fiemap_extent));
fiemap->fm_extent_count = 8

But what will happen? He will only get 4 extent. So do you think it is 
acceptable for a user? We told him that we have 8 extents, he has 
allocated enough space, but he can't get what he wanted. And he need to
fiemap = malloc(sizeof(fiemap) + 16 * sizeof(struct fiemap_extent));
fiemap->fm_extent_count = 16
to get 8 extent for your test file.

> # filefrag -v testfile
> Filesystem type is: 58465342
> File size of testfile is 65536 (16 blocks, blocksize 4096)
>   ext logical physical expected length flags
>     0       1       12               1
>     1       3       14       12      1
>     2       5       16       14      1
>     3       7       18       16      1
>     4       9       20       18      1
>     5      11       22       20      1
>     6      13       24       22      1
>     7      15       26       24      1 eof
> testfile: 9 extents found
> #
>
> FWIW, filefrag seems busted - the file has 8 extents, not 9.
yeah, filefrag is really broken.
>
>> The reason is that in xfs_getbmap we will
>> calculate holes and set it in 'out', while out is malloced by
>> bmv_count(fi_extent_max+1) which didn't consider holes. So in the
>> worst case, if 'out' vector looks like
>> [hole, extent, hole, extent, hole, ... hole, extent, hole],
>> we will only return half of fi_extent_max extents.
>
> Right, it's not broken, we simply return less than fi_extent_mex
> extents when there are holes. I don't see that as a problem as
> applications have to handle that case anyway, and....
see my above test case. I guess we really don't want a userspace user to 
allocate num_extents * 2 + 1 fiemap_extent to get them.
>
>> So in xfs_vn_fiemap, we should consider this worst case. If the
>> user wants fi_extent_max extents, we need a 'out' with size of
>> 2 *fi_extent_max + 2(one more the header).
>
> That's rather dangerous, I think. It relies on other code to catch
> the buffer overrun that this sets up for fragmented, non-sparse
> files. Personally I'd much prefer to return fewer extents for sparse
> files than to add a landmine like this into the kernel code....
We just change the size of our 'out', we don't change fi_extent_max or 
anything related to the fiemap. So I think what we care is how to keep 
our 'out' in good shape and fiemap should handle and check their 
fi_extent_max if we pass it more extents.

btw, maybe there is a better solution for the problem I described above. 
If there is a good one, I am happy to accept it.

Regards,
Tao

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Tao Ma]

On 06/14/2010 08:27 AM, Dave Chinner wrote:
> On Sat, Jun 12, 2010 at 10:08:15AM +0800, Tao Ma wrote:
>> In xfs_vn_fiemap, we set bvm_count to fi_extent_max + 1 and want
>> to return fi_extent_max extents, but actually it won't work for
>> a sparse file.
>
> Define "won't work". i.e. what's the test case?  I just created a
> sparse file and checked it, and it reported all the extents in it:
>
> # xfs_bmap -vp testfile
> testfile:
>   EXT: FILE-OFFSET      BLOCK-RANGE      AG AG-OFFSET        TOTAL FLAGS
>     0: [0..7]:          hole                                     8
>     1: [8..15]:         96..103           0 (96..103)            8 00000
>     2: [16..23]:        hole                                     8
>     3: [24..31]:        112..119          0 (112..119)           8 00000
>     4: [32..39]:        hole                                     8
>     5: [40..47]:        128..135          0 (128..135)           8 00000
>     6: [48..55]:        hole                                     8
>     7: [56..63]:        144..151          0 (144..151)           8 00000
>     8: [64..71]:        hole                                     8
>     9: [72..79]:        160..167          0 (160..167)           8 00000
>    10: [80..87]:        hole                                     8
>    11: [88..95]:        176..183          0 (176..183)           8 00000
>    12: [96..103]:       hole                                     8
>    13: [104..111]:      192..199          0 (192..199)           8 00000
>    14: [112..119]:      hole                                     8
>    15: [120..127]:      208..215          0 (208..215)           8 00000
ok, so let me explain it. In commit 
2d1ff3c75a4642062d314634290be6d8da4ffb03, I add the mode for extent 
query of fiemap for xfs. So with your test file, it will return that we 
have 8 extents(because in xfs_fiemap_format we don't return holes). So 
normally and naturally, a user begin to iterate all the extents by doing

fiemap = malloc(sizeof(fiemap) + 8 * sizeof(struct fiemap_extent));
fiemap->fm_extent_count = 8

But what will happen? He will only get 4 extent. So do you think it is 
acceptable for a user? We told him that we have 8 extents, he has 
allocated enough space, but he can't get what he wanted. And he need to
fiemap = malloc(sizeof(fiemap) + 16 * sizeof(struct fiemap_extent));
fiemap->fm_extent_count = 16
to get 8 extent for your test file.

> # filefrag -v testfile
> Filesystem type is: 58465342
> File size of testfile is 65536 (16 blocks, blocksize 4096)
>   ext logical physical expected length flags
>     0       1       12               1
>     1       3       14       12      1
>     2       5       16       14      1
>     3       7       18       16      1
>     4       9       20       18      1
>     5      11       22       20      1
>     6      13       24       22      1
>     7      15       26       24      1 eof
> testfile: 9 extents found
> #
>
> FWIW, filefrag seems busted - the file has 8 extents, not 9.
yeah, filefrag is really broken.
>
>> The reason is that in xfs_getbmap we will
>> calculate holes and set it in 'out', while out is malloced by
>> bmv_count(fi_extent_max+1) which didn't consider holes. So in the
>> worst case, if 'out' vector looks like
>> [hole, extent, hole, extent, hole, ... hole, extent, hole],
>> we will only return half of fi_extent_max extents.
>
> Right, it's not broken, we simply return less than fi_extent_mex
> extents when there are holes. I don't see that as a problem as
> applications have to handle that case anyway, and....
see my above test case. I guess we really don't want a userspace user to 
allocate num_extents * 2 + 1 fiemap_extent to get them.
>
>> So in xfs_vn_fiemap, we should consider this worst case. If the
>> user wants fi_extent_max extents, we need a 'out' with size of
>> 2 *fi_extent_max + 2(one more the header).
>
> That's rather dangerous, I think. It relies on other code to catch
> the buffer overrun that this sets up for fragmented, non-sparse
> files. Personally I'd much prefer to return fewer extents for sparse
> files than to add a landmine like this into the kernel code....
We just change the size of our 'out', we don't change fi_extent_max or 
anything related to the fiemap. So I think what we care is how to keep 
our 'out' in good shape and fiemap should handle and check their 
fi_extent_max if we pass it more extents.

btw, maybe there is a better solution for the problem I described above. 
If there is a good one, I am happy to accept it.

Regards,
Tao

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Dave Chinner]

On Mon, Jun 14, 2010 at 10:27:06AM +1000, Dave Chinner wrote:
> On Sat, Jun 12, 2010 at 10:08:15AM +0800, Tao Ma wrote:
> > The reason is that in xfs_getbmap we will
> > calculate holes and set it in 'out', while out is malloced by
> > bmv_count(fi_extent_max+1) which didn't consider holes. So in the
> > worst case, if 'out' vector looks like
> > [hole, extent, hole, extent, hole, ... hole, extent, hole],
> > we will only return half of fi_extent_max extents.
> 
> Right, it's not broken, we simply return less than fi_extent_mex
> extents when there are holes. I don't see that as a problem as
> applications have to handle that case anyway, and....
> 
> > So in xfs_vn_fiemap, we should consider this worst case. If the
> > user wants fi_extent_max extents, we need a 'out' with size of
> > 2 *fi_extent_max + 2(one more the header).
> 
> That's rather dangerous, I think. It relies on other code to catch
> the buffer overrun that this sets up for fragmented, non-sparse
> files. Personally I'd much prefer to return fewer extents for sparse
> files than to add a landmine like this into the kernel code....

I just had a thought - if you want to avoid holes being reported to
fiemap, then add a BMV_IF_NO_HOLES flag to xfs_getbmap() and skip
holes in the mappin gloop when this flag is set. That will make
fiemap fill in the full number of extents without hacking the
extent count...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Dave Chinner]

On Mon, Jun 14, 2010 at 10:27:06AM +1000, Dave Chinner wrote:
> On Sat, Jun 12, 2010 at 10:08:15AM +0800, Tao Ma wrote:
> > The reason is that in xfs_getbmap we will
> > calculate holes and set it in 'out', while out is malloced by
> > bmv_count(fi_extent_max+1) which didn't consider holes. So in the
> > worst case, if 'out' vector looks like
> > [hole, extent, hole, extent, hole, ... hole, extent, hole],
> > we will only return half of fi_extent_max extents.
> 
> Right, it's not broken, we simply return less than fi_extent_mex
> extents when there are holes. I don't see that as a problem as
> applications have to handle that case anyway, and....
> 
> > So in xfs_vn_fiemap, we should consider this worst case. If the
> > user wants fi_extent_max extents, we need a 'out' with size of
> > 2 *fi_extent_max + 2(one more the header).
> 
> That's rather dangerous, I think. It relies on other code to catch
> the buffer overrun that this sets up for fragmented, non-sparse
> files. Personally I'd much prefer to return fewer extents for sparse
> files than to add a landmine like this into the kernel code....

I just had a thought - if you want to avoid holes being reported to
fiemap, then add a BMV_IF_NO_HOLES flag to xfs_getbmap() and skip
holes in the mappin gloop when this flag is set. That will make
fiemap fill in the full number of extents without hacking the
extent count...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Tao Ma]

Hi Dave,

Dave Chinner wrote:
> On Mon, Jun 14, 2010 at 10:27:06AM +1000, Dave Chinner wrote:
>   
>> On Sat, Jun 12, 2010 at 10:08:15AM +0800, Tao Ma wrote:
>>     
>>> The reason is that in xfs_getbmap we will
>>> calculate holes and set it in 'out', while out is malloced by
>>> bmv_count(fi_extent_max+1) which didn't consider holes. So in the
>>> worst case, if 'out' vector looks like
>>> [hole, extent, hole, extent, hole, ... hole, extent, hole],
>>> we will only return half of fi_extent_max extents.
>>>       
>> Right, it's not broken, we simply return less than fi_extent_mex
>> extents when there are holes. I don't see that as a problem as
>> applications have to handle that case anyway, and....
>>
>>     
>>> So in xfs_vn_fiemap, we should consider this worst case. If the
>>> user wants fi_extent_max extents, we need a 'out' with size of
>>> 2 *fi_extent_max + 2(one more the header).
>>>       
>> That's rather dangerous, I think. It relies on other code to catch
>> the buffer overrun that this sets up for fragmented, non-sparse
>> files. Personally I'd much prefer to return fewer extents for sparse
>> files than to add a landmine like this into the kernel code....
>>     
>
> I just had a thought - if you want to avoid holes being reported to
> fiemap, then add a BMV_IF_NO_HOLES flag to xfs_getbmap() and skip
> holes in the mappin gloop when this flag is set. That will make
> fiemap fill in the full number of extents without hacking the
> extent count...
>   
yeah, that should work and I will try to generate a patch for it.
I am not quite familiar with xfs, so please be kind to me if I make some 
stupid mistake in the patch. ;)

Regards,
Tao

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Tao Ma]

Hi Dave,

Dave Chinner wrote:
> On Mon, Jun 14, 2010 at 10:27:06AM +1000, Dave Chinner wrote:
>   
>> On Sat, Jun 12, 2010 at 10:08:15AM +0800, Tao Ma wrote:
>>     
>>> The reason is that in xfs_getbmap we will
>>> calculate holes and set it in 'out', while out is malloced by
>>> bmv_count(fi_extent_max+1) which didn't consider holes. So in the
>>> worst case, if 'out' vector looks like
>>> [hole, extent, hole, extent, hole, ... hole, extent, hole],
>>> we will only return half of fi_extent_max extents.
>>>       
>> Right, it's not broken, we simply return less than fi_extent_mex
>> extents when there are holes. I don't see that as a problem as
>> applications have to handle that case anyway, and....
>>
>>     
>>> So in xfs_vn_fiemap, we should consider this worst case. If the
>>> user wants fi_extent_max extents, we need a 'out' with size of
>>> 2 *fi_extent_max + 2(one more the header).
>>>       
>> That's rather dangerous, I think. It relies on other code to catch
>> the buffer overrun that this sets up for fragmented, non-sparse
>> files. Personally I'd much prefer to return fewer extents for sparse
>> files than to add a landmine like this into the kernel code....
>>     
>
> I just had a thought - if you want to avoid holes being reported to
> fiemap, then add a BMV_IF_NO_HOLES flag to xfs_getbmap() and skip
> holes in the mappin gloop when this flag is set. That will make
> fiemap fill in the full number of extents without hacking the
> extent count...
>   
yeah, that should work and I will try to generate a patch for it.
I am not quite familiar with xfs, so please be kind to me if I make some 
stupid mistake in the patch. ;)

Regards,
Tao

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Tao Ma]

Hi Dave,
On 06/14/2010 08:29 PM, Dave Chinner wrote:
> I just had a thought - if you want to avoid holes being reported to
> fiemap, then add a BMV_IF_NO_HOLES flag to xfs_getbmap() and skip
> holes in the mappin gloop when this flag is set. That will make
> fiemap fill in the full number of extents without hacking the
> extent count...
Here is the updated one. I have used BVM_IF_NO_HOLES in xfs_getbmap
to skip increasing index 'cur_ext'. It is a bit ugly, see my commit
log. I guess maybe we can add another flag in xfs_bmapi so that it
don't even give us the holes?

Regards,
Tao

>From cee1765ffd3e2b003b837666b4620b5107ed9ddd Mon Sep 17 00:00:00 2001
From: Tao Ma <tao.ma@oracle.com>
Date: Thu, 17 Jun 2010 16:14:22 +0800
Subject: [PATCH v3] xfs: Make fiemap works with sparse file.

In xfs_vn_fiemap, we set bvm_count to fi_extent_max + 1 and want
to return fi_extent_max extents, but actually it won't work for
a sparse file. The reason is that in xfs_getbmap we will
calculate holes and set it in 'out', while out is malloced by
bmv_count(fi_extent_max+1) which didn't consider holes. So in the
worst case, if 'out' vector looks like
[hole, extent, hole, extent, hole, ... hole, extent, hole],
we will only return half of fi_extent_max extents.

This patch add a new parameter BMV_IF_NO_HOLES for bvm_iflags.
So with this flags, we don't use our 'out' in xfs_getbmap for
a hole. The solution is a bit ugly by just don't increasing
index of 'out' vector. I felt that it is not easy to skip it
at the very beginning since we have the complicated check and
some function like xfs_getbmapx_fix_eof_hole to adjust 'out'.

Cc: Dave Chinner <david@fromorbit.com>
Cc: Alex Elder <aelder@sgi.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/xfs/linux-2.6/xfs_iops.c |    2 +-
 fs/xfs/xfs_bmap.c           |   14 +++++++++++++-
 fs/xfs/xfs_fs.h             |    4 +++-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_iops.c b/fs/xfs/linux-2.6/xfs_iops.c
index 9c8019c..505d5c0 100644
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -677,7 +677,7 @@ xfs_vn_fiemap(
 					fieinfo->fi_extents_max + 1;
 	bm.bmv_count = min_t(__s32, bm.bmv_count,
 			     (PAGE_SIZE * 16 / sizeof(struct getbmapx)));
-	bm.bmv_iflags = BMV_IF_PREALLOC;
+	bm.bmv_iflags = BMV_IF_PREALLOC | BMV_IF_NO_HOLES;
 	if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR)
 		bm.bmv_iflags |= BMV_IF_ATTRFORK;
 	if (!(fieinfo->fi_flags & FIEMAP_FLAG_SYNC))
diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index 99587de..d49107d 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -5744,12 +5744,24 @@ xfs_getbmap(
 					map[i].br_startblock))
 				goto out_free_map;
 
-			nexleft--;
 			bmv->bmv_offset =
 				out[cur_ext].bmv_offset +
 				out[cur_ext].bmv_length;
 			bmv->bmv_length =
 				max_t(__int64_t, 0, bmvend - bmv->bmv_offset);
+
+			/*
+			 * In case we don't want to return the hole,
+			 * don't increase cur_ext so that we can reuse
+			 * it in the next loop.
+			 */
+			if ((iflags & BMV_IF_NO_HOLES) &&
+			    out[cur_ext].bmv_block == -1LL) {
+				memset(&out[cur_ext], 0, sizeof(out[cur_ext]));
+				continue;
+			}
+
+			nexleft--;
 			bmv->bmv_entries++;
 			cur_ext++;
 		}
diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
index 7cf7220..87c2e9d 100644
--- a/fs/xfs/xfs_fs.h
+++ b/fs/xfs/xfs_fs.h
@@ -114,8 +114,10 @@ struct getbmapx {
 #define BMV_IF_NO_DMAPI_READ	0x2	/* Do not generate DMAPI read event  */
 #define BMV_IF_PREALLOC		0x4	/* rtn status BMV_OF_PREALLOC if req */
 #define BMV_IF_DELALLOC		0x8	/* rtn status BMV_OF_DELALLOC if req */
+#define BMV_IF_NO_HOLES		0x10	/* Do not return holes */
 #define BMV_IF_VALID	\
-	(BMV_IF_ATTRFORK|BMV_IF_NO_DMAPI_READ|BMV_IF_PREALLOC|BMV_IF_DELALLOC)
+	(BMV_IF_ATTRFORK|BMV_IF_NO_DMAPI_READ|BMV_IF_PREALLOC|	\
+	 BMV_IF_DELALLOC|BMV_IF_NO_HOLES)
 
 /*	bmv_oflags values - returned for each non-header segment */
 #define BMV_OF_PREALLOC		0x1	/* segment = unwritten pre-allocation */
-- 
1.5.5

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Tao Ma]

Hi Dave,
On 06/14/2010 08:29 PM, Dave Chinner wrote:
> I just had a thought - if you want to avoid holes being reported to
> fiemap, then add a BMV_IF_NO_HOLES flag to xfs_getbmap() and skip
> holes in the mappin gloop when this flag is set. That will make
> fiemap fill in the full number of extents without hacking the
> extent count...
Here is the updated one. I have used BVM_IF_NO_HOLES in xfs_getbmap
to skip increasing index 'cur_ext'. It is a bit ugly, see my commit
log. I guess maybe we can add another flag in xfs_bmapi so that it
don't even give us the holes?

Regards,
Tao

>From cee1765ffd3e2b003b837666b4620b5107ed9ddd Mon Sep 17 00:00:00 2001
From: Tao Ma <tao.ma@oracle.com>
Date: Thu, 17 Jun 2010 16:14:22 +0800
Subject: [PATCH v3] xfs: Make fiemap works with sparse file.

In xfs_vn_fiemap, we set bvm_count to fi_extent_max + 1 and want
to return fi_extent_max extents, but actually it won't work for
a sparse file. The reason is that in xfs_getbmap we will
calculate holes and set it in 'out', while out is malloced by
bmv_count(fi_extent_max+1) which didn't consider holes. So in the
worst case, if 'out' vector looks like
[hole, extent, hole, extent, hole, ... hole, extent, hole],
we will only return half of fi_extent_max extents.

This patch add a new parameter BMV_IF_NO_HOLES for bvm_iflags.
So with this flags, we don't use our 'out' in xfs_getbmap for
a hole. The solution is a bit ugly by just don't increasing
index of 'out' vector. I felt that it is not easy to skip it
at the very beginning since we have the complicated check and
some function like xfs_getbmapx_fix_eof_hole to adjust 'out'.

Cc: Dave Chinner <david@fromorbit.com>
Cc: Alex Elder <aelder@sgi.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/xfs/linux-2.6/xfs_iops.c |    2 +-
 fs/xfs/xfs_bmap.c           |   14 +++++++++++++-
 fs/xfs/xfs_fs.h             |    4 +++-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_iops.c b/fs/xfs/linux-2.6/xfs_iops.c
index 9c8019c..505d5c0 100644
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -677,7 +677,7 @@ xfs_vn_fiemap(
 					fieinfo->fi_extents_max + 1;
 	bm.bmv_count = min_t(__s32, bm.bmv_count,
 			     (PAGE_SIZE * 16 / sizeof(struct getbmapx)));
-	bm.bmv_iflags = BMV_IF_PREALLOC;
+	bm.bmv_iflags = BMV_IF_PREALLOC | BMV_IF_NO_HOLES;
 	if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR)
 		bm.bmv_iflags |= BMV_IF_ATTRFORK;
 	if (!(fieinfo->fi_flags & FIEMAP_FLAG_SYNC))
diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index 99587de..d49107d 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -5744,12 +5744,24 @@ xfs_getbmap(
 					map[i].br_startblock))
 				goto out_free_map;
 
-			nexleft--;
 			bmv->bmv_offset =
 				out[cur_ext].bmv_offset +
 				out[cur_ext].bmv_length;
 			bmv->bmv_length =
 				max_t(__int64_t, 0, bmvend - bmv->bmv_offset);
+
+			/*
+			 * In case we don't want to return the hole,
+			 * don't increase cur_ext so that we can reuse
+			 * it in the next loop.
+			 */
+			if ((iflags & BMV_IF_NO_HOLES) &&
+			    out[cur_ext].bmv_block == -1LL) {
+				memset(&out[cur_ext], 0, sizeof(out[cur_ext]));
+				continue;
+			}
+
+			nexleft--;
 			bmv->bmv_entries++;
 			cur_ext++;
 		}
diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
index 7cf7220..87c2e9d 100644
--- a/fs/xfs/xfs_fs.h
+++ b/fs/xfs/xfs_fs.h
@@ -114,8 +114,10 @@ struct getbmapx {
 #define BMV_IF_NO_DMAPI_READ	0x2	/* Do not generate DMAPI read event  */
 #define BMV_IF_PREALLOC		0x4	/* rtn status BMV_OF_PREALLOC if req */
 #define BMV_IF_DELALLOC		0x8	/* rtn status BMV_OF_DELALLOC if req */
+#define BMV_IF_NO_HOLES		0x10	/* Do not return holes */
 #define BMV_IF_VALID	\
-	(BMV_IF_ATTRFORK|BMV_IF_NO_DMAPI_READ|BMV_IF_PREALLOC|BMV_IF_DELALLOC)
+	(BMV_IF_ATTRFORK|BMV_IF_NO_DMAPI_READ|BMV_IF_PREALLOC|	\
+	 BMV_IF_DELALLOC|BMV_IF_NO_HOLES)
 
 /*	bmv_oflags values - returned for each non-header segment */
 #define BMV_OF_PREALLOC		0x1	/* segment = unwritten pre-allocation */
-- 
1.5.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Dave Chinner]

On Thu, Jun 17, 2010 at 04:53:19PM +0800, Tao Ma wrote:
> Hi Dave,
> On 06/14/2010 08:29 PM, Dave Chinner wrote:
> > I just had a thought - if you want to avoid holes being reported to
> > fiemap, then add a BMV_IF_NO_HOLES flag to xfs_getbmap() and skip
> > holes in the mappin gloop when this flag is set. That will make
> > fiemap fill in the full number of extents without hacking the
> > extent count...
> Here is the updated one. I have used BVM_IF_NO_HOLES in xfs_getbmap
> to skip increasing index 'cur_ext'. It is a bit ugly, see my commit
> log. I guess maybe we can add another flag in xfs_bmapi so that it
> don't even give us the holes?

No need...

> 
> Regards,
> Tao
> 
> From cee1765ffd3e2b003b837666b4620b5107ed9ddd Mon Sep 17 00:00:00 2001
> From: Tao Ma <tao.ma@oracle.com>
> Date: Thu, 17 Jun 2010 16:14:22 +0800
> Subject: [PATCH v3] xfs: Make fiemap works with sparse file.
> 
> In xfs_vn_fiemap, we set bvm_count to fi_extent_max + 1 and want
> to return fi_extent_max extents, but actually it won't work for
> a sparse file. The reason is that in xfs_getbmap we will
> calculate holes and set it in 'out', while out is malloced by
> bmv_count(fi_extent_max+1) which didn't consider holes. So in the
> worst case, if 'out' vector looks like
> [hole, extent, hole, extent, hole, ... hole, extent, hole],
> we will only return half of fi_extent_max extents.
> 
> This patch add a new parameter BMV_IF_NO_HOLES for bvm_iflags.
> So with this flags, we don't use our 'out' in xfs_getbmap for
> a hole. The solution is a bit ugly by just don't increasing
> index of 'out' vector. I felt that it is not easy to skip it
> at the very beginning since we have the complicated check and
> some function like xfs_getbmapx_fix_eof_hole to adjust 'out'.

... because I think we can safely skip xfs_getbmapx_fix_eof_hole()
it only modifies the hole. Hence  just adding a check after the
attribute fork end check (which needs to detect a hole to terminate)
should be fine: e.g something like:

			if (map[i].br_startblock == HOLESTARTBLOCK &&
			    whichfork == XFS_ATTR_FORK) {
				/* came to the end of attribute fork */
				out[cur_ext].bmv_oflags |= BMV_OF_LAST;
				goto out_free_map;
			}
+			if (map[i].br_startblock == HOLESTARTBLOCK &&
+				 (iflags & BMV_IF_NO_HOLES)) {
+				memset(&out[cur_ext], 0, sizeof(out[cur_ext]));
+				continue;
+			}

Should work and avoid the worst of the ugliness.

The rest of the patch looks fine.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Dave Chinner]

On Thu, Jun 17, 2010 at 04:53:19PM +0800, Tao Ma wrote:
> Hi Dave,
> On 06/14/2010 08:29 PM, Dave Chinner wrote:
> > I just had a thought - if you want to avoid holes being reported to
> > fiemap, then add a BMV_IF_NO_HOLES flag to xfs_getbmap() and skip
> > holes in the mappin gloop when this flag is set. That will make
> > fiemap fill in the full number of extents without hacking the
> > extent count...
> Here is the updated one. I have used BVM_IF_NO_HOLES in xfs_getbmap
> to skip increasing index 'cur_ext'. It is a bit ugly, see my commit
> log. I guess maybe we can add another flag in xfs_bmapi so that it
> don't even give us the holes?

No need...

> 
> Regards,
> Tao
> 
> From cee1765ffd3e2b003b837666b4620b5107ed9ddd Mon Sep 17 00:00:00 2001
> From: Tao Ma <tao.ma@oracle.com>
> Date: Thu, 17 Jun 2010 16:14:22 +0800
> Subject: [PATCH v3] xfs: Make fiemap works with sparse file.
> 
> In xfs_vn_fiemap, we set bvm_count to fi_extent_max + 1 and want
> to return fi_extent_max extents, but actually it won't work for
> a sparse file. The reason is that in xfs_getbmap we will
> calculate holes and set it in 'out', while out is malloced by
> bmv_count(fi_extent_max+1) which didn't consider holes. So in the
> worst case, if 'out' vector looks like
> [hole, extent, hole, extent, hole, ... hole, extent, hole],
> we will only return half of fi_extent_max extents.
> 
> This patch add a new parameter BMV_IF_NO_HOLES for bvm_iflags.
> So with this flags, we don't use our 'out' in xfs_getbmap for
> a hole. The solution is a bit ugly by just don't increasing
> index of 'out' vector. I felt that it is not easy to skip it
> at the very beginning since we have the complicated check and
> some function like xfs_getbmapx_fix_eof_hole to adjust 'out'.

... because I think we can safely skip xfs_getbmapx_fix_eof_hole()
it only modifies the hole. Hence  just adding a check after the
attribute fork end check (which needs to detect a hole to terminate)
should be fine: e.g something like:

			if (map[i].br_startblock == HOLESTARTBLOCK &&
			    whichfork == XFS_ATTR_FORK) {
				/* came to the end of attribute fork */
				out[cur_ext].bmv_oflags |= BMV_OF_LAST;
				goto out_free_map;
			}
+			if (map[i].br_startblock == HOLESTARTBLOCK &&
+				 (iflags & BMV_IF_NO_HOLES)) {
+				memset(&out[cur_ext], 0, sizeof(out[cur_ext]));
+				continue;
+			}

Should work and avoid the worst of the ugliness.

The rest of the patch looks fine.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Tao Ma]

Hi Dave,

On 06/18/2010 08:47 AM, Dave Chinner wrote:
> On Thu, Jun 17, 2010 at 04:53:19PM +0800, Tao Ma wrote:
>> Hi Dave,
>> On 06/14/2010 08:29 PM, Dave Chinner wrote:
>>> I just had a thought - if you want to avoid holes being reported to
>>> fiemap, then add a BMV_IF_NO_HOLES flag to xfs_getbmap() and skip
>>> holes in the mappin gloop when this flag is set. That will make
>>> fiemap fill in the full number of extents without hacking the
>>> extent count...
>> Here is the updated one. I have used BVM_IF_NO_HOLES in xfs_getbmap
>> to skip increasing index 'cur_ext'. It is a bit ugly, see my commit
>> log. I guess maybe we can add another flag in xfs_bmapi so that it
>> don't even give us the holes?
>
> No need...
I am fine with it.
>
>>
>> Regards,
>> Tao
>>
>>  From cee1765ffd3e2b003b837666b4620b5107ed9ddd Mon Sep 17 00:00:00 2001
>> From: Tao Ma<tao.ma@oracle.com>
>> Date: Thu, 17 Jun 2010 16:14:22 +0800
>> Subject: [PATCH v3] xfs: Make fiemap works with sparse file.
>>
>> In xfs_vn_fiemap, we set bvm_count to fi_extent_max + 1 and want
>> to return fi_extent_max extents, but actually it won't work for
>> a sparse file. The reason is that in xfs_getbmap we will
>> calculate holes and set it in 'out', while out is malloced by
>> bmv_count(fi_extent_max+1) which didn't consider holes. So in the
>> worst case, if 'out' vector looks like
>> [hole, extent, hole, extent, hole, ... hole, extent, hole],
>> we will only return half of fi_extent_max extents.
>>
>> This patch add a new parameter BMV_IF_NO_HOLES for bvm_iflags.
>> So with this flags, we don't use our 'out' in xfs_getbmap for
>> a hole. The solution is a bit ugly by just don't increasing
>> index of 'out' vector. I felt that it is not easy to skip it
>> at the very beginning since we have the complicated check and
>> some function like xfs_getbmapx_fix_eof_hole to adjust 'out'.
>
> ... because I think we can safely skip xfs_getbmapx_fix_eof_hole()
> it only modifies the hole. Hence  just adding a check after the
> attribute fork end check (which needs to detect a hole to terminate)
> should be fine: e.g something like:
>
> 			if (map[i].br_startblock == HOLESTARTBLOCK&&
> 			whichfork == XFS_ATTR_FORK) {
> 				/* came to the end of attribute fork */
> 				out[cur_ext].bmv_oflags |= BMV_OF_LAST;
> 				goto out_free_map;
> 			}
> +			if (map[i].br_startblock == HOLESTARTBLOCK&&
> +				 (iflags&  BMV_IF_NO_HOLES)) {
> +				memset(&out[cur_ext], 0, sizeof(out[cur_ext]));
> +				continue;
> +			}
>
> Should work and avoid the worst of the ugliness.
I am afraid it doesn't work, at least from my test. It enters a dead loop.
I think the root cause is that your change doesn't update bmv_offset and 
bmv_length for a hole. So in the large loop,
do {
                 nmap = (nexleft > subnex) ? subnex : nexleft;
		error = xfs_bmapi(NULL, ip, XFS_BB_TO_FSBT(mp, bmv->bmv_offset),
                                   XFS_BB_TO_FSB(mp, bmv->bmv_length),
                                   bmapi_flags, NULL, 0, map, &nmap,
                                   NULL, NULL);
                 if (error)
                         goto out_free_map;
...
} while (nmap && nexleft && bmv->bmv_length);

We will dead loop there and we need xfs_getbmapx_fix_eof_hole to go out 
directly.

Regards,
Tao

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Tao Ma]

Hi Dave,

On 06/18/2010 08:47 AM, Dave Chinner wrote:
> On Thu, Jun 17, 2010 at 04:53:19PM +0800, Tao Ma wrote:
>> Hi Dave,
>> On 06/14/2010 08:29 PM, Dave Chinner wrote:
>>> I just had a thought - if you want to avoid holes being reported to
>>> fiemap, then add a BMV_IF_NO_HOLES flag to xfs_getbmap() and skip
>>> holes in the mappin gloop when this flag is set. That will make
>>> fiemap fill in the full number of extents without hacking the
>>> extent count...
>> Here is the updated one. I have used BVM_IF_NO_HOLES in xfs_getbmap
>> to skip increasing index 'cur_ext'. It is a bit ugly, see my commit
>> log. I guess maybe we can add another flag in xfs_bmapi so that it
>> don't even give us the holes?
>
> No need...
I am fine with it.
>
>>
>> Regards,
>> Tao
>>
>>  From cee1765ffd3e2b003b837666b4620b5107ed9ddd Mon Sep 17 00:00:00 2001
>> From: Tao Ma<tao.ma@oracle.com>
>> Date: Thu, 17 Jun 2010 16:14:22 +0800
>> Subject: [PATCH v3] xfs: Make fiemap works with sparse file.
>>
>> In xfs_vn_fiemap, we set bvm_count to fi_extent_max + 1 and want
>> to return fi_extent_max extents, but actually it won't work for
>> a sparse file. The reason is that in xfs_getbmap we will
>> calculate holes and set it in 'out', while out is malloced by
>> bmv_count(fi_extent_max+1) which didn't consider holes. So in the
>> worst case, if 'out' vector looks like
>> [hole, extent, hole, extent, hole, ... hole, extent, hole],
>> we will only return half of fi_extent_max extents.
>>
>> This patch add a new parameter BMV_IF_NO_HOLES for bvm_iflags.
>> So with this flags, we don't use our 'out' in xfs_getbmap for
>> a hole. The solution is a bit ugly by just don't increasing
>> index of 'out' vector. I felt that it is not easy to skip it
>> at the very beginning since we have the complicated check and
>> some function like xfs_getbmapx_fix_eof_hole to adjust 'out'.
>
> ... because I think we can safely skip xfs_getbmapx_fix_eof_hole()
> it only modifies the hole. Hence  just adding a check after the
> attribute fork end check (which needs to detect a hole to terminate)
> should be fine: e.g something like:
>
> 			if (map[i].br_startblock == HOLESTARTBLOCK&&
> 			whichfork == XFS_ATTR_FORK) {
> 				/* came to the end of attribute fork */
> 				out[cur_ext].bmv_oflags |= BMV_OF_LAST;
> 				goto out_free_map;
> 			}
> +			if (map[i].br_startblock == HOLESTARTBLOCK&&
> +				 (iflags&  BMV_IF_NO_HOLES)) {
> +				memset(&out[cur_ext], 0, sizeof(out[cur_ext]));
> +				continue;
> +			}
>
> Should work and avoid the worst of the ugliness.
I am afraid it doesn't work, at least from my test. It enters a dead loop.
I think the root cause is that your change doesn't update bmv_offset and 
bmv_length for a hole. So in the large loop,
do {
                 nmap = (nexleft > subnex) ? subnex : nexleft;
		error = xfs_bmapi(NULL, ip, XFS_BB_TO_FSBT(mp, bmv->bmv_offset),
                                   XFS_BB_TO_FSB(mp, bmv->bmv_length),
                                   bmapi_flags, NULL, 0, map, &nmap,
                                   NULL, NULL);
                 if (error)
                         goto out_free_map;
...
} while (nmap && nexleft && bmv->bmv_length);

We will dead loop there and we need xfs_getbmapx_fix_eof_hole to go out 
directly.

Regards,
Tao

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Dave Chinner]

On Fri, Jun 18, 2010 at 10:27:49AM +0800, Tao Ma wrote:
> >>This patch add a new parameter BMV_IF_NO_HOLES for bvm_iflags.
> >>So with this flags, we don't use our 'out' in xfs_getbmap for
> >>a hole. The solution is a bit ugly by just don't increasing
> >>index of 'out' vector. I felt that it is not easy to skip it
> >>at the very beginning since we have the complicated check and
> >>some function like xfs_getbmapx_fix_eof_hole to adjust 'out'.
> >
> >... because I think we can safely skip xfs_getbmapx_fix_eof_hole()
> >it only modifies the hole. Hence  just adding a check after the
> >attribute fork end check (which needs to detect a hole to terminate)
> >should be fine: e.g something like:
> >
> >			if (map[i].br_startblock == HOLESTARTBLOCK&&
> >			whichfork == XFS_ATTR_FORK) {
> >				/* came to the end of attribute fork */
> >				out[cur_ext].bmv_oflags |= BMV_OF_LAST;
> >				goto out_free_map;
> >			}
> >+			if (map[i].br_startblock == HOLESTARTBLOCK&&
> >+				 (iflags&  BMV_IF_NO_HOLES)) {
> >+				memset(&out[cur_ext], 0, sizeof(out[cur_ext]));
> >+				continue;
> >+			}
> >
> >Should work and avoid the worst of the ugliness.
> I am afraid it doesn't work, at least from my test. It enters a dead loop.
> I think the root cause is that your change doesn't update bmv_offset
> and bmv_length for a hole. So in the large loop,

Yes, you are right - I didn't consider the outer loop termination
properly. My bad.

I'll add your patch to my qa list and move it onwards. Thanks.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Dave Chinner]

On Fri, Jun 18, 2010 at 10:27:49AM +0800, Tao Ma wrote:
> >>This patch add a new parameter BMV_IF_NO_HOLES for bvm_iflags.
> >>So with this flags, we don't use our 'out' in xfs_getbmap for
> >>a hole. The solution is a bit ugly by just don't increasing
> >>index of 'out' vector. I felt that it is not easy to skip it
> >>at the very beginning since we have the complicated check and
> >>some function like xfs_getbmapx_fix_eof_hole to adjust 'out'.
> >
> >... because I think we can safely skip xfs_getbmapx_fix_eof_hole()
> >it only modifies the hole. Hence  just adding a check after the
> >attribute fork end check (which needs to detect a hole to terminate)
> >should be fine: e.g something like:
> >
> >			if (map[i].br_startblock == HOLESTARTBLOCK&&
> >			whichfork == XFS_ATTR_FORK) {
> >				/* came to the end of attribute fork */
> >				out[cur_ext].bmv_oflags |= BMV_OF_LAST;
> >				goto out_free_map;
> >			}
> >+			if (map[i].br_startblock == HOLESTARTBLOCK&&
> >+				 (iflags&  BMV_IF_NO_HOLES)) {
> >+				memset(&out[cur_ext], 0, sizeof(out[cur_ext]));
> >+				continue;
> >+			}
> >
> >Should work and avoid the worst of the ugliness.
> I am afraid it doesn't work, at least from my test. It enters a dead loop.
> I think the root cause is that your change doesn't update bmv_offset
> and bmv_length for a hole. So in the large loop,

Yes, you are right - I didn't consider the outer loop termination
properly. My bad.

I'll add your patch to my qa list and move it onwards. Thanks.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfs: Make fiemap works with sparse file.

[Alex Elder]

On Thu, 2010-06-17 at 16:53 +0800, Tao Ma wrote:
> Hi Dave,
> On 06/14/2010 08:29 PM, Dave Chinner wrote:
> > I just had a thought - if you want to avoid holes being reported to
> > fiemap, then add a BMV_IF_NO_HOLES flag to xfs_getbmap() and skip
> > holes in the mappin gloop when this flag is set. That will make
> > fiemap fill in the full number of extents without hacking the
> > extent count...
> Here is the updated one. I have used BVM_IF_NO_HOLES in xfs_getbmap
> to skip increasing index 'cur_ext'. It is a bit ugly, see my commit
> log. I guess maybe we can add another flag in xfs_bmapi so that it
> don't even give us the holes?

Dave said he would commit this but it hasn't been done
yet, so there's still time to comment!

. . .

> diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
> index 99587de..d49107d 100644
> --- a/fs/xfs/xfs_bmap.c
> +++ b/fs/xfs/xfs_bmap.c
> @@ -5744,12 +5744,24 @@ xfs_getbmap(
>  					map[i].br_startblock))
>  				goto out_free_map;
>  
> -			nexleft--;
>  			bmv->bmv_offset =
>  				out[cur_ext].bmv_offset +
>  				out[cur_ext].bmv_length;
>  			bmv->bmv_length =
>  				max_t(__int64_t, 0, bmvend - bmv->bmv_offset);
> +
> +			/*
> +			 * In case we don't want to return the hole,
> +			 * don't increase cur_ext so that we can reuse
> +			 * it in the next loop.
> +			 */
> +			if ((iflags & BMV_IF_NO_HOLES) &&
> +			    out[cur_ext].bmv_block == -1LL) {

I personally would prefer this to use:
		if ((iflags & BMV_IF_NO_HOLES) &&
			map[i].br_startblock == HOLESTARTBLOCK) {

I think this is more obvious that this "extent" represents
a hole (as opposed to bmv_block == -1, which is how
xfs_getbmapx_fix_eof_hole() encodes this).

Otherwise looks good.

Reviewed-by: Alex Elder <aelder@sgi.com>



> +				memset(&out[cur_ext], 0, sizeof(out[cur_ext]));
> +				continue;
> +			}
> +
> +			nexleft--;
>  			bmv->bmv_entries++;
>  			cur_ext++;
>  		}
> diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
> index 7cf7220..87c2e9d 100644
> --- a/fs/xfs/xfs_fs.h
> +++ b/fs/xfs/xfs_fs.h
> @@ -114,8 +114,10 @@ struct getbmapx {
>  #define BMV_IF_NO_DMAPI_READ	0x2	/* Do not generate DMAPI read event  */
>  #define BMV_IF_PREALLOC		0x4	/* rtn status BMV_OF_PREALLOC if req */
>  #define BMV_IF_DELALLOC		0x8	/* rtn status BMV_OF_DELALLOC if req */
> +#define BMV_IF_NO_HOLES		0x10	/* Do not return holes */
>  #define BMV_IF_VALID	\
> -	(BMV_IF_ATTRFORK|BMV_IF_NO_DMAPI_READ|BMV_IF_PREALLOC|BMV_IF_DELALLOC)
> +	(BMV_IF_ATTRFORK|BMV_IF_NO_DMAPI_READ|BMV_IF_PREALLOC|	\
> +	 BMV_IF_DELALLOC|BMV_IF_NO_HOLES)
>  
>  /*	bmv_oflags values - returned for each non-header segment */
>  #define BMV_OF_PREALLOC		0x1	/* segment = unwritten pre-allocation */



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v4] xfs: Make fiemap works with sparse file.

[Tao Ma]

On 08/28/2010 03:46 AM, Alex Elder wrote:
> On Thu, 2010-06-17 at 16:53 +0800, Tao Ma wrote:
>> >  Hi Dave,
>> >  On 06/14/2010 08:29 PM, Dave Chinner wrote:
>>> >  >  I just had a thought - if you want to avoid holes being reported to
>>> >  >  fiemap, then add a BMV_IF_NO_HOLES flag to xfs_getbmap() and skip
>>> >  >  holes in the mappin gloop when this flag is set. That will make
>>> >  >  fiemap fill in the full number of extents without hacking the
>>> >  >  extent count...
>> >  Here is the updated one. I have used BVM_IF_NO_HOLES in xfs_getbmap
>> >  to skip increasing index 'cur_ext'. It is a bit ugly, see my commit
>> >  log. I guess maybe we can add another flag in xfs_bmapi so that it
>> >  don't even give us the holes?
> Dave said he would commit this but it hasn't been done
> yet, so there's still time to comment!
yeah, it is never late. ;)
> 
> . . .
> 
>> >  diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
>> >  index 99587de..d49107d 100644
>> >  --- a/fs/xfs/xfs_bmap.c
>> >  +++ b/fs/xfs/xfs_bmap.c
>> >  @@ -5744,12 +5744,24 @@ xfs_getbmap(
>> >    					map[i].br_startblock))
>> >    				goto out_free_map;
>> >  
>> >  -			nexleft--;
>> >    			bmv->bmv_offset =
>> >    				out[cur_ext].bmv_offset +
>> >    				out[cur_ext].bmv_length;
>> >    			bmv->bmv_length =
>> >    				max_t(__int64_t, 0, bmvend - bmv->bmv_offset);
>> >  +
>> >  +			/*
>> >  +			 * In case we don't want to return the hole,
>> >  +			 * don't increase cur_ext so that we can reuse
>> >  +			 * it in the next loop.
>> >  +			 */
>> >  +			if ((iflags&  BMV_IF_NO_HOLES)&&
>> >  +			    out[cur_ext].bmv_block == -1LL) {
> I personally would prefer this to use:
> 		if ((iflags&  BMV_IF_NO_HOLES)&&
> 			map[i].br_startblock == HOLESTARTBLOCK) {
> 
> I think this is more obvious that this "extent" represents
> a hole (as opposed to bmv_block == -1, which is how
> xfs_getbmapx_fix_eof_hole() encodes this).
> 
> Otherwise looks good.
OK, I have updated the patch and attached it. So could you please send
it to Linus?

Regards,
Tao


>From 09a3cbaa456b35ed071cdd08a8b3c4c129baf451 Mon Sep 17 00:00:00 2001
From: Tao Ma <tao.ma@oracle.com>
Date: Mon, 30 Aug 2010 10:04:51 +0800
Subject: [PATCH v4] xfs: Make fiemap works with sparse file.

In xfs_vn_fiemap, we set bvm_count to fi_extent_max + 1 and want
to return fi_extent_max extents, but actually it won't work for
a sparse file. The reason is that in xfs_getbmap we will
calculate holes and set it in 'out', while out is malloced by
bmv_count(fi_extent_max+1) which didn't consider holes. So in the
worst case, if 'out' vector looks like
[hole, extent, hole, extent, hole, ... hole, extent, hole],
we will only return half of fi_extent_max extents.

This patch add a new parameter BMV_IF_NO_HOLES for bvm_iflags.
So with this flags, we don't use our 'out' in xfs_getbmap for
a hole. The solution is a bit ugly by just don't increasing
index of 'out' vector. I felt that it is not easy to skip it
at the very beginning since we have the complicated check and
some function like xfs_getbmapx_fix_eof_hole to adjust 'out'.

Cc: Dave Chinner <david@fromorbit.com>
Reviewed-by: Alex Elder <aelder@sgi.com>
Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/xfs/linux-2.6/xfs_iops.c |    2 +-
 fs/xfs/xfs_bmap.c           |   14 +++++++++++++-
 fs/xfs/xfs_fs.h             |    4 +++-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_iops.c b/fs/xfs/linux-2.6/xfs_iops.c
index 68be25d..b1fc2a6 100644
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -664,7 +664,7 @@ xfs_vn_fiemap(
 					fieinfo->fi_extents_max + 1;
 	bm.bmv_count = min_t(__s32, bm.bmv_count,
 			     (PAGE_SIZE * 16 / sizeof(struct getbmapx)));
-	bm.bmv_iflags = BMV_IF_PREALLOC;
+	bm.bmv_iflags = BMV_IF_PREALLOC | BMV_IF_NO_HOLES;
 	if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR)
 		bm.bmv_iflags |= BMV_IF_ATTRFORK;
 	if (!(fieinfo->fi_flags & FIEMAP_FLAG_SYNC))
diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index 23f14e5..f90dadd 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -5533,12 +5533,24 @@ xfs_getbmap(
 					map[i].br_startblock))
 				goto out_free_map;
 
-			nexleft--;
 			bmv->bmv_offset =
 				out[cur_ext].bmv_offset +
 				out[cur_ext].bmv_length;
 			bmv->bmv_length =
 				max_t(__int64_t, 0, bmvend - bmv->bmv_offset);
+
+			/*
+			 * In case we don't want to return the hole,
+			 * don't increase cur_ext so that we can reuse
+			 * it in the next loop.
+			 */
+			if ((iflags & BMV_IF_NO_HOLES) &&
+			    map[i].br_startblock == HOLESTARTBLOCK) {
+				memset(&out[cur_ext], 0, sizeof(out[cur_ext]));
+				continue;
+			}
+
+			nexleft--;
 			bmv->bmv_entries++;
 			cur_ext++;
 		}
diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
index 7cf7220..87c2e9d 100644
--- a/fs/xfs/xfs_fs.h
+++ b/fs/xfs/xfs_fs.h
@@ -114,8 +114,10 @@ struct getbmapx {
 #define BMV_IF_NO_DMAPI_READ	0x2	/* Do not generate DMAPI read event  */
 #define BMV_IF_PREALLOC		0x4	/* rtn status BMV_OF_PREALLOC if req */
 #define BMV_IF_DELALLOC		0x8	/* rtn status BMV_OF_DELALLOC if req */
+#define BMV_IF_NO_HOLES		0x10	/* Do not return holes */
 #define BMV_IF_VALID	\
-	(BMV_IF_ATTRFORK|BMV_IF_NO_DMAPI_READ|BMV_IF_PREALLOC|BMV_IF_DELALLOC)
+	(BMV_IF_ATTRFORK|BMV_IF_NO_DMAPI_READ|BMV_IF_PREALLOC|	\
+	 BMV_IF_DELALLOC|BMV_IF_NO_HOLES)
 
 /*	bmv_oflags values - returned for each non-header segment */
 #define BMV_OF_PREALLOC		0x1	/* segment = unwritten pre-allocation */
-- 
1.7.1.571.gba4d01

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs