Expose the number of hole blocks in a file #7392
Conversation
There was a problem hiding this comment.
Thanks! I'd still like to get fiemap implemented at some point since it's a much richer interface. But it sounds like this is enough for your use case and there's no issue with filesystem specific ioctls.
Along with this change we should add a basic test case to verify it's working. I thought OpenZFS already had one we could adapt for this but I wasn't able to find it.
| @@ -888,6 +888,38 @@ zpl_ioctl_setxattr(struct file *filp, void __user *arg) | |||
| return (err); | |||
| } | |||
|
|
|||
| static long | |||
| zpl_ioctl_count_filled(struct file *filep, void __user *arg) | |||
There was a problem hiding this comment.
For the most part we've tried to keep the Linux specific changes confined to the zpl_* functions, and left the core ZFS functionality in zfs_vnops.c, zfs_vfsops.c, etc. So let's go ahead and split this function up the same way. Lines 901-914 can be moved in to a zfs_count_filled() function in zfs_vnops.c and then zpl_ioctl_count_filled is what's left. This way most of the linux data structures, functions, negative return codes don't seep down very far in to ZFS proper.
static long
zpl_ioctl_count_filled(struct file *filep, void __user *arg)
{
struct inode *ip = file_inode(filep);
zfsvfs_t *zfsvfs = ITOZSB(ip);
dmu_object_info_t doi;
int error;
error = -zfs_count_filled(zfsvfs, ITOZ(ip), &doi);
if (error == 0) {
uint64_t ndata = doi.doi_fill_count;
error = copy_to_user(arg, &ndata, sizeof (ndata));
}
return (error);
}There was a problem hiding this comment.
Makes sense. I adjusted the interface you suggested slightly:
int zfs_count_filled(struct inode *ip, uint64_t *fill_count)
lib/libzfs/libzfs_util.c
Outdated
| return (err); | ||
| } | ||
|
|
||
| *count = (ss.st_size + ss.st_blksize - 1) / ss.st_blksize - fill; |
There was a problem hiding this comment.
Might be wise to guard against a dividing by zero here even if st_blksize should always be non-zero.
include/sys/fs/zfs.h
Outdated
| /* | ||
| * zpl ioctl to get number of filled blocks | ||
| */ | ||
| #define ZFS_IOC_COUNT_FILLED _IOR('f', 100, uint64_t) |
There was a problem hiding this comment.
Rather than using the global 'f' magic here, each filesystem is allowed to use their own unique magic for custom ioctl()'s to prevent collisions with the generic ones. This will be our first for ZFS so we'll need to pick a magic value, how about 'Z' which looks to be unused. We should also then be able to start at 1.
#define ZFS_IOCTL_MAGIC 'Z'
#define ZFS_IOC_COUNT_FILLED _IOR(ZFS_IOCTL_MAGIC, 1, uint64_t)
There was a problem hiding this comment.
According to linux/Documentation/ioctl/ioctl-number.txt the Z space is in (limited) use:
'Z' 14-15 drivers/message/fusion/mptctl.h
I'd suggest to start at 32 or 64 to avoid potential conflicts.
There was a problem hiding this comment.
I changed it to 'Z' but kept it at number 100.
Codecov Report
@@ Coverage Diff @@
## master #7392 +/- ##
==========================================
+ Coverage 77.26% 77.5% +0.24%
==========================================
Files 336 338 +2
Lines 107520 107757 +237
==========================================
+ Hits 83072 83516 +444
+ Misses 24448 24241 -207
Continue to review full report at Codecov.
|
|
These are the hole tests I wasn't able to find previously. They were never ported to ZoL since this was a custom |
|
For Matt's reference, the ZFS FIEMAP feature is described in issue #264, and more generally in fiemap.txt. While there are some issues that are ZFS specific that are not implemented in the current IMHO, it would be preferable to have a single interface and user tool for this kind of information, rather than having multiple different tools. Note that even if ZFS doesn't implement the full FIEMAP interface, it could provide a similar interface by calling and return |
include/sys/fs/zfs.h
Outdated
| /* | ||
| * zpl ioctl to get number of filled blocks | ||
| */ | ||
| #define ZFS_IOC_COUNT_FILLED _IOR('f', 100, uint64_t) |
There was a problem hiding this comment.
According to linux/Documentation/ioctl/ioctl-number.txt the Z space is in (limited) use:
'Z' 14-15 drivers/message/fusion/mptctl.h
I'd suggest to start at 32 or 64 to avoid potential conflicts.
lib/libzfs/libzfs_util.c
Outdated
| * calculated. | ||
| * | ||
| * On success, zero is returned, the count argument is set to the | ||
| * number of holes, and the bs argument is set to the block size (if it is |
There was a problem hiding this comment.
Out of curiosity, it isn't clear how the blocksize is useful here, though I guess it doesn't add significantly to the overhead? This is already returned by stat() for the file, and it isn't like blocksize * number_of_holes is a useful value? Hmm, maybe the confusion is that "the number of holes" is actually "the number of unallocated blocks" and not "the number of contiguous unallocated regions" (which is what I'd consider the traditional definition of "hole" is).
I guess in that case blocksize is useful, but the comment (and description of this feature in general) should be improved. Rather than describe this as "number of holes", I'd write "number of unallocated (sparse) blocks inside a file".
In that case FS_IOC_FIEMAP with fm_extent_count = 0 is unfortunately not a suitable replacement for this functionality.
There was a problem hiding this comment.
That's covered in the first part of the comment, but I'll reiterate it here too
/*
* zfs_get_hole_count retrieves the number of holes (blocks which are
* zero-filled) in the specified file using the ZFS_IOC_COUNT_FILLED ioctl.
module/zfs/dmu.c
Outdated
| * ID and wait for that to be synced. | ||
| */ | ||
| int | ||
| dmu_object_wait_synced(objset_t *os, uint64_t object) |
There was a problem hiding this comment.
This is actually a function that would be useful for Lustre also, to be able to sync a specific dnode instead of the whole filesystem, but it can be a no-op if the dnode does not have dirty pages.
module/zfs/zpl_file.c
Outdated
| error = copy_to_user(arg, &ndata, sizeof (ndata)); | ||
| ZFS_EXIT(zfsvfs); | ||
| return (error); | ||
|
|
There was a problem hiding this comment.
empty line should be above return()?
module/zfs/dmu.c
Outdated
| int error, i; | ||
|
|
||
| error = dnode_hold(os, object, FTAG, &dn); | ||
| if (error) { |
There was a problem hiding this comment.
Far be it from me to criticize your ZFS coding style, but it doesn't seem common even in the ZFS code to have single-line "if" blocks with {} (and also on line 2364). Please ignore if incorrect.
There was a problem hiding this comment.
Yep, I'll go ahead and change these.
|
I've made the changes requested, except for adding the tests, which I'll get to next week. |
|
Added the hole tests and rebased. |
|
Looks like you need to add the new |
|
@behlendorf thanks - I had been testing by running just the new tests. |
|
It looks like CentOS 6 doesn't have SEEK_DATA/HOLE. I can use |
We do. What we've done historically is something like this in if ! getholes /etc/hosts; then
log_unsupported "The kernel does not support SEEK_DATA / SEEK_HOLE"
fi |
|
@behlendorf Turns out that doesn't work, becuase the builders don't run ZFS on root. But the test is failing in a bizarre way on CentOS 6: |
We would like to expose the number of hole (sparse) blocks in a file. This can be useful to for example if you want to fill in the holes with some data; knowing the number of holes in advances allows you to report progress on hole filling. We could use SEEK_HOLE to do that but it would be O(n) where n is the number of holes present in the file.
|
test failures: CentOS 6: bizarre, test scripts are missing Ubuntu 18.04: doesn't seem related to my changes (and they pass on my ubuntu 18.04 machine) CentOS 7: doesn't seem related to my changes; correctly skips the holes tests |
|
@behlendorf Given that the fiemap changes haven't landed in the intervening 3 years, would you be open to adding this functionality as-is? Or is there a smaller change than the fiemap stuff that would make this acceptable? If not, I'll close this PR. |
|
@behlendorf replied to me out-of-band and is concerned about adding a new custom ioctl that we'd have to support forever. We'll close this PR and continue maintaining the diffs in our branch indefinitely. |
Description
We would like to expose the number of hole (sparse) blocks in a file.
This can be useful to for example if you want to fill in the holes with
some data; knowing the number of holes in advances allows you to report
progress on hole filling. We could use SEEK_HOLE to do that but it would
be O(n) where n is the number of holes present in the file.
We would like to get feedback on this approach - is this an OK way to do it, and if so what additional work is needed to get it integrated.
Work by @grwilson
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist:
Signed-off-by.