* [PATCH 1/4] fs: add SEEK_HOLE and SEEK_DATA flags
@ 2011-06-27 18:02 Josef Bacik
2011-06-27 18:02 ` [PATCH 2/4] Btrfs: implement our own ->llseek Josef Bacik
` (4 more replies)
0 siblings, 5 replies; 74+ messages in thread
From: Josef Bacik @ 2011-06-27 18:02 UTC (permalink / raw)
To: linux-fsdevel, linux-btrfs, linux-kernel, viro
This just gets us ready to support the SEEK_HOLE and SEEK_DATA flags. Turns out
using fiemap in things like cp cause more problems than it solves, so lets try
and give userspace an interface that doesn't suck. We need to match solaris
here, and the definitions are
*o* If /whence/ is SEEK_HOLE, the offset of the start of the
next hole greater than or equal to the supplied offset
is returned. The definition of a hole is provided near
the end of the DESCRIPTION.
*o* If /whence/ is SEEK_DATA, the file pointer is set to the
start of the next non-hole file region greater than or
equal to the supplied offset.
So in the generic case the entire file is data and there is a virtual hole at
the end. That means we will just return i_size for SEEK_HOLE and will return
the same offset for SEEK_DATA. This is how Solaris does it so we have to do it
the same way.
Thanks,
Signed-off-by: Josef Bacik <josef@redhat.com>
---
fs/read_write.c | 37 +++++++++++++++++++++++++++++++++++++
include/linux/fs.h | 4 +++-
2 files changed, 40 insertions(+), 1 deletions(-)
diff --git a/fs/read_write.c b/fs/read_write.c
index 5520f8a..d58551e 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -64,6 +64,23 @@ generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin)
return file->f_pos;
offset += file->f_pos;
break;
+ case SEEK_DATA:
+ /*
+ * In the generic case the entire file is data, so as long as
+ * offset isn't at the end of the file then the offset is data.
+ */
+ if (offset >= inode->i_size)
+ return -ENXIO;
+ break;
+ case SEEK_HOLE:
+ /*
+ * There is a virtual hole at the end of the file, so as long as
+ * offset isn't i_size or larger, return i_size.
+ */
+ if (offset >= inode->i_size)
+ return -ENXIO;
+ offset = inode->i_size;
+ break;
}
if (offset < 0 && !unsigned_offsets(file))
@@ -141,6 +158,26 @@ loff_t default_llseek(struct file *file, loff_t offset, int origin)
goto out;
}
offset += file->f_pos;
+ break;
+ case SEEK_DATA:
+ /*
+ * In the generic case the entire file is data, so as
+ * long as offset isn't at the end of the file then the
+ * offset is data.
+ */
+ if (offset >= inode->i_size)
+ return -ENXIO;
+ break;
+ case SEEK_HOLE:
+ /*
+ * There is a virtual hole at the end of the file, so
+ * as long as offset isn't i_size or larger, return
+ * i_size.
+ */
+ if (offset >= inode->i_size)
+ return -ENXIO;
+ offset = inode->i_size;
+ break;
}
retval = -EINVAL;
if (offset >= 0 || unsigned_offsets(file)) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6e73e2e..552cdfe 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -32,7 +32,9 @@
#define SEEK_SET 0 /* seek relative to beginning of file */
#define SEEK_CUR 1 /* seek relative to current file position */
#define SEEK_END 2 /* seek relative to end of file */
-#define SEEK_MAX SEEK_END
+#define SEEK_DATA 3 /* seek to the next data */
+#define SEEK_HOLE 4 /* seek to the next hole */
+#define SEEK_MAX SEEK_HOLE
struct fstrim_range {
__u64 start;
--
1.7.5.2
^ permalink raw reply related [flat|nested] 74+ messages in thread* [PATCH 2/4] Btrfs: implement our own ->llseek 2011-06-27 18:02 [PATCH 1/4] fs: add SEEK_HOLE and SEEK_DATA flags Josef Bacik @ 2011-06-27 18:02 ` Josef Bacik 2011-06-27 18:02 ` [PATCH 3/4] Ext4: handle SEEK_HOLE/SEEK_DATA generically Josef Bacik ` (3 subsequent siblings) 4 siblings, 0 replies; 74+ messages in thread From: Josef Bacik @ 2011-06-27 18:02 UTC (permalink / raw) To: linux-fsdevel, linux-btrfs, linux-kernel, viro In order to handle SEEK_HOLE/SEEK_DATA we need to implement our own llseek. Basically for the normal SEEK_*'s we will just defer to the generic helper, and for SEEK_HOLE/SEEK_DATA we will use our fiemap helper to figure out the nearest hole or data. Currently this helper doesn't check for delalloc bytes for prealloc space, so for now treat prealloc as data until that is fixed. Thanks, Signed-off-by: Josef Bacik <josef@redhat.com> --- fs/btrfs/ctree.h | 3 + fs/btrfs/file.c | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 150 insertions(+), 1 deletions(-) diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 3006287..8040f4b 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -2506,6 +2506,9 @@ int btrfs_csum_truncate(struct btrfs_trans_handle *trans, int btrfs_lookup_csums_range(struct btrfs_root *root, u64 start, u64 end, struct list_head *list, int search_commit); /* inode.c */ +struct extent_map *btrfs_get_extent_fiemap(struct inode *inode, struct page *page, + size_t pg_offset, u64 start, u64 len, + int create); /* RHEL and EL kernels have a patch that renames PG_checked to FsMisc */ #if defined(ClearPageFsMisc) && !defined(ClearPageChecked) diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index fa4ef18..bd4d061 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -1664,8 +1664,154 @@ out: return ret; } +static int find_desired_extent(struct inode *inode, loff_t *offset, int origin) +{ + struct btrfs_root *root = BTRFS_I(inode)->root; + struct extent_map *em; + struct extent_state *cached_state = NULL; + u64 lockstart = *offset; + u64 lockend = i_size_read(inode); + u64 start = *offset; + u64 orig_start = *offset; + u64 len = i_size_read(inode); + u64 last_end = 0; + int ret = 0; + + lockend = max_t(u64, root->sectorsize, lockend); + if (lockend <= lockstart) + lockend = lockstart + root->sectorsize; + + len = lockend - lockstart + 1; + + len = max_t(u64, len, root->sectorsize); + if (inode->i_size == 0) + return -ENXIO; + + lock_extent_bits(&BTRFS_I(inode)->io_tree, lockstart, lockend, 0, + &cached_state, GFP_NOFS); + + /* + * Delalloc is such a pain. If we have a hole and we have pending + * delalloc for a portion of the hole we will get back a hole that + * exists for the entire range since it hasn't been actually written + * yet. So to take care of this case we need to look for an extent just + * before the position we want in case there is outstanding delalloc + * going on here. + */ + if (origin == SEEK_HOLE && start != 0) { + if (start <= root->sectorsize) + em = btrfs_get_extent_fiemap(inode, NULL, 0, 0, + root->sectorsize, 0); + else + em = btrfs_get_extent_fiemap(inode, NULL, 0, + start - root->sectorsize, + root->sectorsize, 0); + if (IS_ERR(em)) { + ret = -ENXIO; + goto out; + } + last_end = em->start + em->len; + if (em->block_start == EXTENT_MAP_DELALLOC) + last_end = min_t(u64, last_end, inode->i_size); + free_extent_map(em); + } + + while (1) { + em = btrfs_get_extent_fiemap(inode, NULL, 0, start, len, 0); + if (IS_ERR(em)) { + ret = -ENXIO; + break; + } + + if (em->block_start == EXTENT_MAP_HOLE) { + if (test_bit(EXTENT_FLAG_VACANCY, &em->flags)) { + if (last_end <= orig_start) { + free_extent_map(em); + ret = -ENXIO; + break; + } + } + + if (origin == SEEK_HOLE) { + *offset = start; + free_extent_map(em); + break; + } + } else { + if (origin == SEEK_DATA) { + if (em->block_start == EXTENT_MAP_DELALLOC) { + if (start >= inode->i_size) { + free_extent_map(em); + ret = -ENXIO; + break; + } + } + + *offset = start; + free_extent_map(em); + break; + } + } + + start = em->start + em->len; + last_end = em->start + em->len; + + if (em->block_start == EXTENT_MAP_DELALLOC) + last_end = min_t(u64, last_end, inode->i_size); + + if (test_bit(EXTENT_FLAG_VACANCY, &em->flags)) { + free_extent_map(em); + ret = -ENXIO; + break; + } + free_extent_map(em); + cond_resched(); + } + if (!ret) + *offset = min(*offset, inode->i_size); +out: + unlock_extent_cached(&BTRFS_I(inode)->io_tree, lockstart, lockend, + &cached_state, GFP_NOFS); + return ret; +} + +static loff_t btrfs_file_llseek(struct file *file, loff_t offset, int origin) +{ + struct inode *inode = file->f_mapping->host; + int ret; + + mutex_lock(&inode->i_mutex); + switch (origin) { + case SEEK_END: + case SEEK_CUR: + offset = generic_file_llseek_unlocked(file, offset, origin); + goto out; + case SEEK_DATA: + case SEEK_HOLE: + ret = find_desired_extent(inode, &offset, origin); + if (ret) { + mutex_unlock(&inode->i_mutex); + return ret; + } + } + + if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) + return -EINVAL; + if (offset > inode->i_sb->s_maxbytes) + return -EINVAL; + + /* Special lock needed here? */ + if (offset != file->f_pos) { + file->f_pos = offset; + file->f_version = 0; + } +out: + mutex_unlock(&inode->i_mutex); + return offset; +} + const struct file_operations btrfs_file_operations = { - .llseek = generic_file_llseek, + .llseek = btrfs_file_llseek, .read = do_sync_read, .write = do_sync_write, .aio_read = generic_file_aio_read, -- 1.7.5.2 ^ permalink raw reply related [flat|nested] 74+ messages in thread
* [PATCH 3/4] Ext4: handle SEEK_HOLE/SEEK_DATA generically 2011-06-27 18:02 [PATCH 1/4] fs: add SEEK_HOLE and SEEK_DATA flags Josef Bacik 2011-06-27 18:02 ` [PATCH 2/4] Btrfs: implement our own ->llseek Josef Bacik @ 2011-06-27 18:02 ` Josef Bacik 2011-06-27 18:02 ` [PATCH 4/4] fs: handle SEEK_HOLE/SEEK_DATA properly in all fs's that define their own llseek Josef Bacik ` (2 subsequent siblings) 4 siblings, 0 replies; 74+ messages in thread From: Josef Bacik @ 2011-06-27 18:02 UTC (permalink / raw) To: linux-fsdevel, linux-btrfs, linux-kernel, viro Since Ext4 has its own lseek we need to make sure it handles SEEK_HOLE/SEEK_DATA. For now just do the same thing that is done in the generic case, somebody else can come along and make it do fancy things later. Thanks, Signed-off-by: Josef Bacik <josef@redhat.com> --- fs/ext4/file.c | 21 +++++++++++++++++++++ 1 files changed, 21 insertions(+), 0 deletions(-) diff --git a/fs/ext4/file.c b/fs/ext4/file.c index 2c09723..ce766f9 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -236,6 +236,27 @@ loff_t ext4_llseek(struct file *file, loff_t offset, int origin) } offset += file->f_pos; break; + case SEEK_DATA: + /* + * In the generic case the entire file is data, so as long as + * offset isn't at the end of the file then the offset is data. + */ + if (offset >= inode->i_size) { + mutex_unlock(&inode->i_mutex); + return -ENXIO; + } + break; + case SEEK_HOLE: + /* + * There is a virtual hole at the end of the file, so as long as + * offset isn't i_size or larger, return i_size. + */ + if (offset >= inode->i_size) { + mutex_unlock(&inode->i_mutex); + return -ENXIO; + } + offset = inode->i_size; + break; } if (offset < 0 || offset > maxbytes) { -- 1.7.5.2 ^ permalink raw reply related [flat|nested] 74+ messages in thread
* [PATCH 4/4] fs: handle SEEK_HOLE/SEEK_DATA properly in all fs's that define their own llseek 2011-06-27 18:02 [PATCH 1/4] fs: add SEEK_HOLE and SEEK_DATA flags Josef Bacik 2011-06-27 18:02 ` [PATCH 2/4] Btrfs: implement our own ->llseek Josef Bacik 2011-06-27 18:02 ` [PATCH 3/4] Ext4: handle SEEK_HOLE/SEEK_DATA generically Josef Bacik @ 2011-06-27 18:02 ` Josef Bacik 2011-06-27 18:02 ` [PATCH] xfstests 255: add a seek_data/seek_hole tester Josef Bacik 2011-06-27 21:04 ` [PATCH 1/4] fs: add SEEK_HOLE and SEEK_DATA flags Josef Bacik 4 siblings, 0 replies; 74+ messages in thread From: Josef Bacik @ 2011-06-27 18:02 UTC (permalink / raw) To: linux-fsdevel, linux-btrfs, linux-kernel, viro This converts everybody to handle SEEK_HOLE/SEEK_DATA properly. In some cases we just return -EINVAL, in others we do the normal generic thing, and in others we're simply making sure that the properly due-dilligence is done. For example in NFS/CIFS we need to make sure the file size is update properly for the SEEK_HOLE and SEEK_DATA case, but since it calls the generic llseek stuff itself that is all we have to do. Thanks, Signed-off-by: Josef Bacik <josef@redhat.com> --- fs/block_dev.c | 11 ++++++++--- fs/ceph/dir.c | 8 +++++++- fs/ceph/file.c | 20 ++++++++++++++++++-- fs/cifs/cifsfs.c | 7 +++++-- fs/fuse/file.c | 21 +++++++++++++++++++-- fs/hpfs/dir.c | 4 ++++ fs/nfs/file.c | 7 +++++-- 7 files changed, 66 insertions(+), 12 deletions(-) diff --git a/fs/block_dev.c b/fs/block_dev.c index 1a2421f..29fde2c 100644 --- a/fs/block_dev.c +++ b/fs/block_dev.c @@ -355,20 +355,25 @@ static loff_t block_llseek(struct file *file, loff_t offset, int origin) mutex_lock(&bd_inode->i_mutex); size = i_size_read(bd_inode); + retval = -EINVAL; switch (origin) { - case 2: + case SEEK_END: offset += size; break; - case 1: + case SEEK_CUR: offset += file->f_pos; + case SEEK_SET: + break; + default: + goto out; } - retval = -EINVAL; if (offset >= 0 && offset <= size) { if (offset != file->f_pos) { file->f_pos = offset; } retval = offset; } +out: mutex_unlock(&bd_inode->i_mutex); return retval; } diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index ef8f08c..79cd77c 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -446,14 +446,19 @@ static loff_t ceph_dir_llseek(struct file *file, loff_t offset, int origin) loff_t retval; mutex_lock(&inode->i_mutex); + retval = -EINVAL; switch (origin) { case SEEK_END: offset += inode->i_size + 2; /* FIXME */ break; case SEEK_CUR: offset += file->f_pos; + case SEEK_SET: + break; + default: + goto out; } - retval = -EINVAL; + if (offset >= 0 && offset <= inode->i_sb->s_maxbytes) { if (offset != file->f_pos) { file->f_pos = offset; @@ -477,6 +482,7 @@ static loff_t ceph_dir_llseek(struct file *file, loff_t offset, int origin) if (offset > old_offset) fi->dir_release_count--; } +out: mutex_unlock(&inode->i_mutex); return retval; } diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 9542f07..774feb1 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -770,13 +770,16 @@ static loff_t ceph_llseek(struct file *file, loff_t offset, int origin) mutex_lock(&inode->i_mutex); __ceph_do_pending_vmtruncate(inode); - switch (origin) { - case SEEK_END: + if (origin != SEEK_CUR || origin != SEEK_SET) { ret = ceph_do_getattr(inode, CEPH_STAT_CAP_SIZE); if (ret < 0) { offset = ret; goto out; } + } + + switch (origin) { + case SEEK_END: offset += inode->i_size; break; case SEEK_CUR: @@ -792,6 +795,19 @@ static loff_t ceph_llseek(struct file *file, loff_t offset, int origin) } offset += file->f_pos; break; + case SEEK_DATA: + if (offset >= inode->i_size) { + ret = -ENXIO; + goto out; + } + break; + case SEEK_HOLE: + if (offset >= inode->i_size) { + ret = -ENXIO; + goto out; + } + offset = inode->i_size; + break; } if (offset < 0 || offset > inode->i_sb->s_maxbytes) { diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index 2f0c586..acaaf62 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -772,8 +772,11 @@ static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov, static loff_t cifs_llseek(struct file *file, loff_t offset, int origin) { - /* origin == SEEK_END => we must revalidate the cached file length */ - if (origin == SEEK_END) { + /* + * origin == SEEK_END || SEEK_DATA || SEEK_HOLE => we must revalidate + * the cached file length + */ + if (origin != SEEK_SET || origin != SEEK_CUR) { int rc; struct inode *inode = file->f_path.dentry->d_inode; diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 82a6646..73b89df 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -1600,15 +1600,32 @@ static loff_t fuse_file_llseek(struct file *file, loff_t offset, int origin) struct inode *inode = file->f_path.dentry->d_inode; mutex_lock(&inode->i_mutex); - switch (origin) { - case SEEK_END: + if (origin != SEEK_CUR || origin != SEEK_SET) { retval = fuse_update_attributes(inode, NULL, file, NULL); if (retval) goto exit; + } + + switch (origin) { + case SEEK_END: offset += i_size_read(inode); break; case SEEK_CUR: offset += file->f_pos; + break; + case SEEK_DATA: + if (offset >= i_size_read(inode)) { + retval = -ENXIO; + goto exit; + } + break; + case SEEK_HOLE: + if (offset >= i_size_read(inode)) { + retval = -ENXIO; + goto exit; + } + offset = i_size_read(inode); + break; } retval = -EINVAL; if (offset >= 0 && offset <= inode->i_sb->s_maxbytes) { diff --git a/fs/hpfs/dir.c b/fs/hpfs/dir.c index f46ae02..96a8ed9 100644 --- a/fs/hpfs/dir.c +++ b/fs/hpfs/dir.c @@ -29,6 +29,10 @@ static loff_t hpfs_dir_lseek(struct file *filp, loff_t off, int whence) struct hpfs_inode_info *hpfs_inode = hpfs_i(i); struct super_block *s = i->i_sb; + /* Somebody else will have to figure out what to do here */ + if (whence == SEEK_DATA || whence == SEEK_HOLE) + return -EINVAL; + hpfs_lock(s); /*printk("dir lseek\n");*/ diff --git a/fs/nfs/file.c b/fs/nfs/file.c index 2f093ed..2c1705b 100644 --- a/fs/nfs/file.c +++ b/fs/nfs/file.c @@ -187,8 +187,11 @@ static loff_t nfs_file_llseek(struct file *filp, loff_t offset, int origin) filp->f_path.dentry->d_name.name, offset, origin); - /* origin == SEEK_END => we must revalidate the cached file length */ - if (origin == SEEK_END) { + /* + * origin == SEEK_END || SEEK_DATA || SEEK_HOLE => we must revalidate + * the cached file length + */ + if (origin != SEEK_SET || origin != SEEK_CUR) { struct inode *inode = filp->f_mapping->host; int retval = nfs_revalidate_file_size(inode, filp); -- 1.7.5.2 ^ permalink raw reply related [flat|nested] 74+ messages in thread
* [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-27 18:02 [PATCH 1/4] fs: add SEEK_HOLE and SEEK_DATA flags Josef Bacik ` (2 preceding siblings ...) 2011-06-27 18:02 ` [PATCH 4/4] fs: handle SEEK_HOLE/SEEK_DATA properly in all fs's that define their own llseek Josef Bacik @ 2011-06-27 18:02 ` Josef Bacik 2011-06-27 18:32 ` Andreas Dilger 2011-06-27 21:04 ` [PATCH 1/4] fs: add SEEK_HOLE and SEEK_DATA flags Josef Bacik 4 siblings, 1 reply; 74+ messages in thread From: Josef Bacik @ 2011-06-27 18:02 UTC (permalink / raw) To: linux-fsdevel, linux-btrfs, linux-kernel, viro This is a test to make sure seek_data/seek_hole is acting like it does on Solaris. It will check to see if the fs supports finding a hole or not and will adjust as necessary. Cc: xfs@oss.sgi.com Signed-off-by: Josef Bacik <josef@redhat.com> --- 255 | 71 ++++++++ 255.out | 2 + src/seek-tester.c | 473 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 546 insertions(+), 0 deletions(-) create mode 100755 255 create mode 100644 255.out create mode 100644 src/seek-tester.c diff --git a/255 b/255 new file mode 100755 index 0000000..4bb4d0b --- /dev/null +++ b/255 @@ -0,0 +1,71 @@ +#! /bin/bash +# FS QA Test No. 255 +# +# Test SEEK_DATA and SEEK_HOLE +# +#----------------------------------------------------------------------- +# Copyright (c) 2011 Red Hat. All Rights Reserved. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +# +#----------------------------------------------------------------------- +# +# creator +owner=josef@redhat.com + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! + +_cleanup() +{ + rm -f $tmp.* +} + +trap "_cleanup ; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +# real QA test starts here +_supported_fs generic +_supported_os Linux + +testfile=$TEST_DIR/seek_test.$$ +logfile=$TEST_DIR/seek_test.$$.log + +[ -x $here/src/seek-tester ] || _notrun "seek-tester not built" + +_cleanup() +{ + rm -f $testfile + rm -f $logfile +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +echo "Silence is golden" +$here/src/seek-tester -q $testfile 2>&1 | tee -a $logfile + +if grep -q "SEEK_HOLE is not supported" $logfile; then + _notrun "SEEK_HOLE/SEEK_DATA not supported by this kernel" +fi + +rm -f $logfile +rm -f $testfile + +status=0 ; exit diff --git a/255.out b/255.out new file mode 100644 index 0000000..7eefb82 --- /dev/null +++ b/255.out @@ -0,0 +1,2 @@ +QA output created by 255 +Silence is golden diff --git a/src/seek-tester.c b/src/seek-tester.c new file mode 100644 index 0000000..2b8c957 --- /dev/null +++ b/src/seek-tester.c @@ -0,0 +1,473 @@ +/* + * Copyright (C) 2011 Oracle. All rights reserved. + * Copyright (C) 2011 Red Hat. All rights reserved. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public + * License v2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public + * License along with this program; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place - Suite 330, + * Boston, MA 021110-1307, USA. + */ + +#define _XOPEN_SOURCE 500 +#include <sys/types.h> +#include <sys/stat.h> +#include <errno.h> +#include <fcntl.h> +#include <stdio.h> +#include <string.h> +#include <unistd.h> +#include <stdlib.h> + +#define SEEK_DATA 3 +#define SEEK_HOLE 4 + +#define FS_NO_HOLES (1 << 0) +#define QUIET (1 << 1) + +static blksize_t alloc_size; +static unsigned flags = 0; + +static int get_io_sizes(int fd) +{ + struct stat buf; + int ret; + + ret = fstat(fd, &buf); + if (ret) + fprintf(stderr, " ERROR %d: Failed to find io blocksize\n", + errno); + + /* st_blksize is typically also the allocation size */ + alloc_size = buf.st_blksize; + + if (!(flags & QUIET)) + printf("Allocation size: %ld\n", alloc_size); + + return ret; +} + +#define do_free(x) do { if(x) free(x); } while(0); + +static void *do_malloc(size_t size) +{ + void *buf; + + buf = malloc(size); + if (!buf) + fprintf(stderr, " ERROR: Unable to allocate %ld bytes\n", + (long)size); + + return buf; +} + +static int do_truncate(int fd, off_t length) +{ + int ret; + + ret = ftruncate(fd, length); + if (ret) + fprintf(stderr, " ERROR %d: Failed to extend file " + "to %ld bytes\n", errno, (long)length); + return ret; +} + +static ssize_t do_pwrite(int fd, const void *buf, size_t count, off_t offset) +{ + ssize_t ret, written = 0; + + while (count > written) { + ret = pwrite(fd, buf + written, count - written, offset + written); + if (ret < 0) { + fprintf(stderr, " ERROR %d: Failed to write %ld " + "bytes\n", errno, (long)count); + return ret; + } + written += ret; + } + + return 0; +} + +static int do_lseek(int testnum, int subtest, int fd, int origin, off_t set, + off_t exp) +{ + off_t pos; + int ret = -1; + + pos = lseek(fd, set, origin); + + if (pos != exp) { + fprintf(stderr, " ERROR in Test %d.%d: POS expected %ld, " + "got %ld\n", testnum, subtest, (long)exp, (long)pos); + goto out; + } + + if (pos == -1 && errno != ENXIO) { + fprintf(stderr, " ERROR in Test %d.%d: ERRNO expected %d, " + "got %d\n", testnum, subtest, ENXIO, errno); + goto out; + } + + ret = 0; + +out: + return ret; +} + +static int get_flags(int fd) +{ + const char *buf = "ABCDEF"; + ssize_t written; + off_t pos; + int ret; + + ret = do_truncate(fd, alloc_size * 2); + if (ret) + return ret; + + written = do_pwrite(fd, buf, strlen(buf), 0); + if (written) + return -1; + + pos = lseek(fd, 0, SEEK_HOLE); + if (pos == alloc_size * 2) { + if (!(flags & QUIET)) + printf("File system does not recognize holes, the only " + "hole found will be at the end.\n"); + flags |= FS_NO_HOLES; + } else if (pos == (off_t)-1) { + fprintf(stderr, "SEEK_HOLE is not supported\n"); + return -1; + } + + return 0; +} + +/* test hole data hole data */ +static int test06(int fd, int testnum) +{ + int ret = 0; + char *buf = NULL; + int bufsz = alloc_size; + int filsz = bufsz * 4; + int off; + + if (flags & FS_NO_HOLES) + return 1; + + /* HOLE - DATA - HOLE - DATA */ + /* Each unit is bufsz */ + + buf = do_malloc(bufsz); + if (!buf) + goto out; + memset(buf, 'a', bufsz); + + ret = do_pwrite(fd, buf, bufsz, bufsz); + if (!ret) + do_pwrite(fd, buf, bufsz, bufsz * 3); + if (ret) + goto out; + + /* offset at the beginning */ + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, 0); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, 1); + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, bufsz); + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, bufsz); + + /* offset around first hole-data boundary */ + off = bufsz; + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, off - 1, off - 1); + ret += do_lseek(testnum, 6, fd, SEEK_DATA, off - 1, off); + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, off, bufsz * 2); + ret += do_lseek(testnum, 8, fd, SEEK_DATA, off, off); + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, off + 1, bufsz * 2); + ret += do_lseek(testnum, 10, fd, SEEK_DATA, off + 1, off + 1); + + /* offset around data-hole boundary */ + off = bufsz * 2; + ret += do_lseek(testnum, 11, fd, SEEK_HOLE, off - 1, off); + ret += do_lseek(testnum, 12, fd, SEEK_DATA, off - 1, off - 1); + ret += do_lseek(testnum, 13, fd, SEEK_HOLE, off, off); + ret += do_lseek(testnum, 14, fd, SEEK_DATA, off, bufsz * 3); + ret += do_lseek(testnum, 15, fd, SEEK_HOLE, off + 1, off + 1); + ret += do_lseek(testnum, 16, fd, SEEK_DATA, off + 1, bufsz * 3); + + /* offset around second hole-data boundary */ + off = bufsz * 3; + ret += do_lseek(testnum, 17, fd, SEEK_HOLE, off - 1, off - 1); + ret += do_lseek(testnum, 18, fd, SEEK_DATA, off - 1, off); + ret += do_lseek(testnum, 19, fd, SEEK_HOLE, off, filsz); + ret += do_lseek(testnum, 20, fd, SEEK_DATA, off, off); + ret += do_lseek(testnum, 21, fd, SEEK_HOLE, off + 1, filsz); + ret += do_lseek(testnum, 22, fd, SEEK_DATA, off + 1, off + 1); + + /* offset around the end of file */ + off = filsz; + ret += do_lseek(testnum, 23, fd, SEEK_HOLE, off - 1, filsz); + ret += do_lseek(testnum, 24, fd, SEEK_DATA, off - 1, filsz - 1); + ret += do_lseek(testnum, 25, fd, SEEK_HOLE, off, -1); + ret += do_lseek(testnum, 26, fd, SEEK_DATA, off, -1); + ret += do_lseek(testnum, 27, fd, SEEK_HOLE, off + 1, -1); + ret += do_lseek(testnum, 28, fd, SEEK_DATA, off + 1, -1); + +out: + do_free(buf); + return ret; +} + +/* test file with data at the beginning and a hole at the end */ +static int test05(int fd, int testnum) +{ + int ret = -1; + char *buf = NULL; + int bufsz = alloc_size; + int filsz = bufsz * 4; + + if (flags & FS_NO_HOLES) + return 1; + + /* DATA - HOLE */ + /* Each unit is bufsz */ + + buf = do_malloc(bufsz); + if (!buf) + goto out; + memset(buf, 'a', bufsz); + + ret = do_truncate(fd, filsz); + if (!ret) + ret = do_pwrite(fd, buf, bufsz, 0); + if (ret) + goto out; + + /* offset at the beginning */ + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, bufsz); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, bufsz); + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, 0); + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, 1); + + /* offset around data-hole boundary */ + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, bufsz - 1, bufsz); + ret += do_lseek(testnum, 6, fd, SEEK_DATA, bufsz - 1, bufsz - 1); + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, bufsz, bufsz); + ret += do_lseek(testnum, 8, fd, SEEK_DATA, bufsz, -1); + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, bufsz + 1, bufsz + 1); + ret += do_lseek(testnum, 10, fd, SEEK_DATA, bufsz + 1, -1); + + /* offset around eof */ + ret += do_lseek(testnum, 11, fd, SEEK_HOLE, filsz - 1, filsz - 1); + ret += do_lseek(testnum, 12, fd, SEEK_DATA, filsz - 1, -1); + ret += do_lseek(testnum, 13, fd, SEEK_HOLE, filsz, -1); + ret += do_lseek(testnum, 14, fd, SEEK_DATA, filsz, -1); + ret += do_lseek(testnum, 15, fd, SEEK_HOLE, filsz + 1, -1); + ret += do_lseek(testnum, 16, fd, SEEK_DATA, filsz + 1, -1); + +out: + do_free(buf); + return ret; +} + +/* test hole begin and data end */ +static int test04(int fd, int testnum) +{ + int ret; + char *buf = "ABCDEFGH"; + int bufsz = sizeof(buf); + int holsz = alloc_size * 2; + int filsz = holsz + bufsz; + + if (flags & FS_NO_HOLES) + return 1; + + /* HOLE - DATA */ + + ret = do_pwrite(fd, buf, bufsz, holsz); + if (ret) + goto out; + + /* offset at the beginning */ + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, 0); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, 1); + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, holsz); + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, holsz); + + /* offset around hole-data boundary */ + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, holsz - 1, holsz - 1); + ret += do_lseek(testnum, 6, fd, SEEK_DATA, holsz - 1, holsz); + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, holsz, filsz); + ret += do_lseek(testnum, 8, fd, SEEK_DATA, holsz, holsz); + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, holsz + 1, filsz); + ret += do_lseek(testnum, 10, fd, SEEK_DATA, holsz + 1, holsz + 1); + + /* offset around eof */ + ret += do_lseek(testnum, 11, fd, SEEK_HOLE, filsz - 1, filsz); + ret += do_lseek(testnum, 12, fd, SEEK_DATA, filsz - 1, filsz - 1); + ret += do_lseek(testnum, 13, fd, SEEK_HOLE, filsz, -1); + ret += do_lseek(testnum, 14, fd, SEEK_DATA, filsz, -1); + ret += do_lseek(testnum, 15, fd, SEEK_HOLE, filsz + 1, -1); + ret += do_lseek(testnum, 16, fd, SEEK_DATA, filsz + 1, -1); +out: + return ret; +} + +/* test full file */ +static int test03(int fd, int testnum) +{ + char *buf = NULL; + int bufsz = alloc_size + 100; + int ret = -1; + + buf = do_malloc(bufsz); + if (!buf) + goto out; + memset(buf, 'a', bufsz); + + ret = do_pwrite(fd, buf, bufsz, 0); + if (ret) + goto out; + + /* offset at the beginning */ + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, bufsz); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, bufsz); + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, 0); + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, 1); + + /* offset around eof */ + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, bufsz - 1, bufsz); + ret += do_lseek(testnum, 6, fd, SEEK_DATA, bufsz - 1, bufsz - 1); + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, bufsz, -1); + ret += do_lseek(testnum, 8, fd, SEEK_DATA, bufsz, -1); + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, bufsz + 1, -1); + ret += do_lseek(testnum, 10, fd, SEEK_DATA, bufsz + 1, -1); + +out: + do_free(buf); + return ret; +} + +/* test empty file */ +static int test02(int fd, int testnum) +{ + int ret = 0; + + ret += do_lseek(testnum, 1, fd, SEEK_DATA, 0, -1); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 0, -1); + ret += do_lseek(testnum, 3, fd, SEEK_HOLE, 1, -1); + + return ret; +} + +/* test feature support */ +static int test01(int fd, int testnum) +{ + int ret; + char buf[] = "ABCDEFGH"; + int bufsz = sizeof(buf); + + ret = do_pwrite(fd, buf, bufsz, 0); + if (ret) + goto out; + + ret += do_lseek(testnum, 1, fd, SEEK_DATA, 0, 0); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 0, bufsz); + +out: + return ret; +} + +struct testrec { + int test_num; + int (*test_func)(int fd, int testnum); + char *test_desc; +}; + +struct testrec seek_tests[] = { + { 1, test01, "Test basic support" }, + { 2, test02, "Test an empty file" }, + { 3, test03, "Test a full file" }, + { 4, test04, "Test file hole at beg, data at end" }, + { 5, test05, "Test file data at beg, hole at end" }, + { 6, test06, "Test file hole data hole data" }, +}; + +static int run_test(int fd, struct testrec *tr) +{ + int ret; + + ret = tr->test_func(fd, tr->test_num); + if (!(flags & QUIET)) + printf("%02d. %-50s\t%s\n", tr->test_num, tr->test_desc, + ret < 0 ? "FAIL" : (ret == 0 ? "SUCC" : "NOT RUN")); + return ret; +} + +void print_help() +{ + printf("seek-test [-h] [-q] filename\n"); + printf("\t-h - this message\n"); + printf("\t-q - quiet, no output\n"); + printf("\tfilename - file to use for the test\n"); +} + +int main(int argc, char **argv) +{ + int ret = -1; + int i, fd = -1; + int c; + int numtests = sizeof(seek_tests) / sizeof(struct testrec); + + while ((c = getopt(argc, argv, "qh")) != -1) { + switch (c) { + case 'q': + flags |= QUIET; + break; + case 'h': + print_help(); + exit(0); + default: + print_help(); + exit(1); + } + } + + if (optind >= argc) { + print_help(); + exit(1); + } + + fd = open(argv[optind], O_RDWR|O_CREAT|O_TRUNC, 0644); + if (fd < 0) { + fprintf(stderr, "Failed to open testfile: %d\n", errno); + goto out; + } + + ret = get_io_sizes(fd); + if (ret) + goto out; + + ret = get_flags(fd); + if (ret) + goto out; + + for (i = 0; i < numtests; ++i) { + ret = do_truncate(fd, 0); + if (ret) + goto out; + run_test(fd, &seek_tests[i]); + } + +out: + if (fd > -1) + close(fd); + return ret; +} -- 1.7.5.2 ^ permalink raw reply related [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-27 18:02 ` [PATCH] xfstests 255: add a seek_data/seek_hole tester Josef Bacik @ 2011-06-27 18:32 ` Andreas Dilger 2011-06-27 18:47 ` Josef Bacik 0 siblings, 1 reply; 74+ messages in thread From: Andreas Dilger @ 2011-06-27 18:32 UTC (permalink / raw) To: Josef Bacik; +Cc: linux-fsdevel, linux-btrfs, linux-kernel, viro On 2011-06-27, at 12:02 PM, Josef Bacik wrote: > This is a test to make sure seek_data/seek_hole is acting like it does on > Solaris. It will check to see if the fs supports finding a hole or not and will > adjust as necessary. > > diff --git a/src/seek-tester.c b/src/seek-tester.c > new file mode 100644 > index 0000000..2b8c957 > --- /dev/null > +++ b/src/seek-tester.c > @@ -0,0 +1,473 @@ > +/* > + * Copyright (C) 2011 Oracle. All rights reserved. > + * Copyright (C) 2011 Red Hat. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public > + * License v2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + * > + * You should have received a copy of the GNU General Public > + * License along with this program; if not, write to the > + * Free Software Foundation, Inc., 59 Temple Place - Suite 330, > + * Boston, MA 021110-1307, USA. > + */ > + > +#define _XOPEN_SOURCE 500 > +#include <sys/types.h> > +#include <sys/stat.h> > +#include <errno.h> > +#include <fcntl.h> > +#include <stdio.h> > +#include <string.h> > +#include <unistd.h> > +#include <stdlib.h> > + > +#define SEEK_DATA 3 > +#define SEEK_HOLE 4 These should probably be "#ifndef SEEK_DATA" so that gcc doesn't complain in the future when these are added to a standard header. > +#define FS_NO_HOLES (1 << 0) > +#define QUIET (1 << 1) > + > +static blksize_t alloc_size; > +static unsigned flags = 0; > + > +static int get_io_sizes(int fd) > +{ > + struct stat buf; > + int ret; > + > + ret = fstat(fd, &buf); > + if (ret) > + fprintf(stderr, " ERROR %d: Failed to find io blocksize\n", > + errno); > + > + /* st_blksize is typically also the allocation size */ > + alloc_size = buf.st_blksize; > + > + if (!(flags & QUIET)) > + printf("Allocation size: %ld\n", alloc_size); > + > + return ret; > +} > + > +#define do_free(x) do { if(x) free(x); } while(0); > + > +static void *do_malloc(size_t size) > +{ > + void *buf; > + > + buf = malloc(size); > + if (!buf) > + fprintf(stderr, " ERROR: Unable to allocate %ld bytes\n", > + (long)size); > + > + return buf; > +} > + > +static int do_truncate(int fd, off_t length) > +{ > + int ret; > + > + ret = ftruncate(fd, length); > + if (ret) > + fprintf(stderr, " ERROR %d: Failed to extend file " > + "to %ld bytes\n", errno, (long)length); > + return ret; > +} > + > +static ssize_t do_pwrite(int fd, const void *buf, size_t count, off_t offset) > +{ > + ssize_t ret, written = 0; > + > + while (count > written) { > + ret = pwrite(fd, buf + written, count - written, offset + written); > + if (ret < 0) { > + fprintf(stderr, " ERROR %d: Failed to write %ld " > + "bytes\n", errno, (long)count); > + return ret; > + } > + written += ret; > + } > + > + return 0; > +} > + > +static int do_lseek(int testnum, int subtest, int fd, int origin, off_t set, > + off_t exp) > +{ > + off_t pos; > + int ret = -1; > + > + pos = lseek(fd, set, origin); > + > + if (pos != exp) { > + fprintf(stderr, " ERROR in Test %d.%d: POS expected %ld, " > + "got %ld\n", testnum, subtest, (long)exp, (long)pos); > + goto out; > + } > + > + if (pos == -1 && errno != ENXIO) { > + fprintf(stderr, " ERROR in Test %d.%d: ERRNO expected %d, " > + "got %d\n", testnum, subtest, ENXIO, errno); > + goto out; > + } > + > + ret = 0; > + > +out: > + return ret; > +} > + > +static int get_flags(int fd) > +{ > + const char *buf = "ABCDEF"; > + ssize_t written; > + off_t pos; > + int ret; > + > + ret = do_truncate(fd, alloc_size * 2); > + if (ret) > + return ret; > + > + written = do_pwrite(fd, buf, strlen(buf), 0); > + if (written) > + return -1; > + > + pos = lseek(fd, 0, SEEK_HOLE); > + if (pos == alloc_size * 2) { > + if (!(flags & QUIET)) > + printf("File system does not recognize holes, the only " > + "hole found will be at the end.\n"); > + flags |= FS_NO_HOLES; This is a question that I've also had about compatibility with older (well, every) Linux kernel that does not support SEEK_{HOLE,DATA} today. My reading of the existing generic_file_llseek() and default_llseek() code, along with most filesystem-specific llseek() implementations is that they will happily ignore the @whence parameter if it is not known, and pretend like it is 0 (SEEK_SET), so they will just set the position to the @offset parameter and return this value. In that case, the above "SEEK_HOLE" test would incorrectly fail on every Linux kernel in existence today because the returned pos == 0. Should applications call both SEEK_HOLE and SEEK_DATA with @offset=0, and if they return the same values (which is normally impossible, decide that the kernel does not support this SEEK_* functionality? > + } else if (pos == (off_t)-1) { > + fprintf(stderr, "SEEK_HOLE is not supported\n"); > + return -1; > + } > + > + return 0; > +} > + > +/* test hole data hole data */ > +static int test06(int fd, int testnum) > +{ > + int ret = 0; > + char *buf = NULL; > + int bufsz = alloc_size; > + int filsz = bufsz * 4; > + int off; > + > + if (flags & FS_NO_HOLES) > + return 1; > + > + /* HOLE - DATA - HOLE - DATA */ > + /* Each unit is bufsz */ > + > + buf = do_malloc(bufsz); > + if (!buf) > + goto out; > + memset(buf, 'a', bufsz); > + > + ret = do_pwrite(fd, buf, bufsz, bufsz); > + if (!ret) > + do_pwrite(fd, buf, bufsz, bufsz * 3); > + if (ret) > + goto out; > + > + /* offset at the beginning */ > + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, 0); > + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, 1); > + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, bufsz); > + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, bufsz); > + > + /* offset around first hole-data boundary */ > + off = bufsz; > + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, off - 1, off - 1); > + ret += do_lseek(testnum, 6, fd, SEEK_DATA, off - 1, off); > + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, off, bufsz * 2); > + ret += do_lseek(testnum, 8, fd, SEEK_DATA, off, off); > + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, off + 1, bufsz * 2); > + ret += do_lseek(testnum, 10, fd, SEEK_DATA, off + 1, off + 1); > + > + /* offset around data-hole boundary */ > + off = bufsz * 2; > + ret += do_lseek(testnum, 11, fd, SEEK_HOLE, off - 1, off); > + ret += do_lseek(testnum, 12, fd, SEEK_DATA, off - 1, off - 1); > + ret += do_lseek(testnum, 13, fd, SEEK_HOLE, off, off); > + ret += do_lseek(testnum, 14, fd, SEEK_DATA, off, bufsz * 3); > + ret += do_lseek(testnum, 15, fd, SEEK_HOLE, off + 1, off + 1); > + ret += do_lseek(testnum, 16, fd, SEEK_DATA, off + 1, bufsz * 3); > + > + /* offset around second hole-data boundary */ > + off = bufsz * 3; > + ret += do_lseek(testnum, 17, fd, SEEK_HOLE, off - 1, off - 1); > + ret += do_lseek(testnum, 18, fd, SEEK_DATA, off - 1, off); > + ret += do_lseek(testnum, 19, fd, SEEK_HOLE, off, filsz); > + ret += do_lseek(testnum, 20, fd, SEEK_DATA, off, off); > + ret += do_lseek(testnum, 21, fd, SEEK_HOLE, off + 1, filsz); > + ret += do_lseek(testnum, 22, fd, SEEK_DATA, off + 1, off + 1); > + > + /* offset around the end of file */ > + off = filsz; > + ret += do_lseek(testnum, 23, fd, SEEK_HOLE, off - 1, filsz); > + ret += do_lseek(testnum, 24, fd, SEEK_DATA, off - 1, filsz - 1); > + ret += do_lseek(testnum, 25, fd, SEEK_HOLE, off, -1); > + ret += do_lseek(testnum, 26, fd, SEEK_DATA, off, -1); > + ret += do_lseek(testnum, 27, fd, SEEK_HOLE, off + 1, -1); > + ret += do_lseek(testnum, 28, fd, SEEK_DATA, off + 1, -1); > + > +out: > + do_free(buf); > + return ret; > +} > + > +/* test file with data at the beginning and a hole at the end */ > +static int test05(int fd, int testnum) > +{ > + int ret = -1; > + char *buf = NULL; > + int bufsz = alloc_size; > + int filsz = bufsz * 4; > + > + if (flags & FS_NO_HOLES) > + return 1; > + > + /* DATA - HOLE */ > + /* Each unit is bufsz */ > + > + buf = do_malloc(bufsz); > + if (!buf) > + goto out; > + memset(buf, 'a', bufsz); > + > + ret = do_truncate(fd, filsz); > + if (!ret) > + ret = do_pwrite(fd, buf, bufsz, 0); > + if (ret) > + goto out; > + > + /* offset at the beginning */ > + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, bufsz); > + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, bufsz); > + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, 0); > + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, 1); > + > + /* offset around data-hole boundary */ > + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, bufsz - 1, bufsz); > + ret += do_lseek(testnum, 6, fd, SEEK_DATA, bufsz - 1, bufsz - 1); > + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, bufsz, bufsz); > + ret += do_lseek(testnum, 8, fd, SEEK_DATA, bufsz, -1); > + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, bufsz + 1, bufsz + 1); > + ret += do_lseek(testnum, 10, fd, SEEK_DATA, bufsz + 1, -1); > + > + /* offset around eof */ > + ret += do_lseek(testnum, 11, fd, SEEK_HOLE, filsz - 1, filsz - 1); > + ret += do_lseek(testnum, 12, fd, SEEK_DATA, filsz - 1, -1); > + ret += do_lseek(testnum, 13, fd, SEEK_HOLE, filsz, -1); > + ret += do_lseek(testnum, 14, fd, SEEK_DATA, filsz, -1); > + ret += do_lseek(testnum, 15, fd, SEEK_HOLE, filsz + 1, -1); > + ret += do_lseek(testnum, 16, fd, SEEK_DATA, filsz + 1, -1); > + > +out: > + do_free(buf); > + return ret; > +} > + > +/* test hole begin and data end */ > +static int test04(int fd, int testnum) > +{ > + int ret; > + char *buf = "ABCDEFGH"; > + int bufsz = sizeof(buf); > + int holsz = alloc_size * 2; > + int filsz = holsz + bufsz; > + > + if (flags & FS_NO_HOLES) > + return 1; > + > + /* HOLE - DATA */ > + > + ret = do_pwrite(fd, buf, bufsz, holsz); > + if (ret) > + goto out; > + > + /* offset at the beginning */ > + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, 0); > + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, 1); > + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, holsz); > + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, holsz); > + > + /* offset around hole-data boundary */ > + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, holsz - 1, holsz - 1); > + ret += do_lseek(testnum, 6, fd, SEEK_DATA, holsz - 1, holsz); > + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, holsz, filsz); > + ret += do_lseek(testnum, 8, fd, SEEK_DATA, holsz, holsz); > + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, holsz + 1, filsz); > + ret += do_lseek(testnum, 10, fd, SEEK_DATA, holsz + 1, holsz + 1); > + > + /* offset around eof */ > + ret += do_lseek(testnum, 11, fd, SEEK_HOLE, filsz - 1, filsz); > + ret += do_lseek(testnum, 12, fd, SEEK_DATA, filsz - 1, filsz - 1); > + ret += do_lseek(testnum, 13, fd, SEEK_HOLE, filsz, -1); > + ret += do_lseek(testnum, 14, fd, SEEK_DATA, filsz, -1); > + ret += do_lseek(testnum, 15, fd, SEEK_HOLE, filsz + 1, -1); > + ret += do_lseek(testnum, 16, fd, SEEK_DATA, filsz + 1, -1); > +out: > + return ret; > +} > + > +/* test full file */ > +static int test03(int fd, int testnum) > +{ > + char *buf = NULL; > + int bufsz = alloc_size + 100; > + int ret = -1; > + > + buf = do_malloc(bufsz); > + if (!buf) > + goto out; > + memset(buf, 'a', bufsz); > + > + ret = do_pwrite(fd, buf, bufsz, 0); > + if (ret) > + goto out; > + > + /* offset at the beginning */ > + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, bufsz); > + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, bufsz); > + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, 0); > + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, 1); > + > + /* offset around eof */ > + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, bufsz - 1, bufsz); > + ret += do_lseek(testnum, 6, fd, SEEK_DATA, bufsz - 1, bufsz - 1); > + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, bufsz, -1); > + ret += do_lseek(testnum, 8, fd, SEEK_DATA, bufsz, -1); > + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, bufsz + 1, -1); > + ret += do_lseek(testnum, 10, fd, SEEK_DATA, bufsz + 1, -1); > + > +out: > + do_free(buf); > + return ret; > +} > + > +/* test empty file */ > +static int test02(int fd, int testnum) > +{ > + int ret = 0; > + > + ret += do_lseek(testnum, 1, fd, SEEK_DATA, 0, -1); > + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 0, -1); > + ret += do_lseek(testnum, 3, fd, SEEK_HOLE, 1, -1); > + > + return ret; > +} > + > +/* test feature support */ > +static int test01(int fd, int testnum) > +{ > + int ret; > + char buf[] = "ABCDEFGH"; > + int bufsz = sizeof(buf); > + > + ret = do_pwrite(fd, buf, bufsz, 0); > + if (ret) > + goto out; > + > + ret += do_lseek(testnum, 1, fd, SEEK_DATA, 0, 0); > + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 0, bufsz); > + > +out: > + return ret; > +} > + > +struct testrec { > + int test_num; > + int (*test_func)(int fd, int testnum); > + char *test_desc; > +}; > + > +struct testrec seek_tests[] = { > + { 1, test01, "Test basic support" }, > + { 2, test02, "Test an empty file" }, > + { 3, test03, "Test a full file" }, > + { 4, test04, "Test file hole at beg, data at end" }, > + { 5, test05, "Test file data at beg, hole at end" }, > + { 6, test06, "Test file hole data hole data" }, > +}; > + > +static int run_test(int fd, struct testrec *tr) > +{ > + int ret; > + > + ret = tr->test_func(fd, tr->test_num); > + if (!(flags & QUIET)) > + printf("%02d. %-50s\t%s\n", tr->test_num, tr->test_desc, > + ret < 0 ? "FAIL" : (ret == 0 ? "SUCC" : "NOT RUN")); > + return ret; > +} > + > +void print_help() > +{ > + printf("seek-test [-h] [-q] filename\n"); > + printf("\t-h - this message\n"); > + printf("\t-q - quiet, no output\n"); > + printf("\tfilename - file to use for the test\n"); > +} > + > +int main(int argc, char **argv) > +{ > + int ret = -1; > + int i, fd = -1; > + int c; > + int numtests = sizeof(seek_tests) / sizeof(struct testrec); > + > + while ((c = getopt(argc, argv, "qh")) != -1) { > + switch (c) { > + case 'q': > + flags |= QUIET; > + break; > + case 'h': > + print_help(); > + exit(0); > + default: > + print_help(); > + exit(1); > + } > + } > + > + if (optind >= argc) { > + print_help(); > + exit(1); > + } > + > + fd = open(argv[optind], O_RDWR|O_CREAT|O_TRUNC, 0644); > + if (fd < 0) { > + fprintf(stderr, "Failed to open testfile: %d\n", errno); > + goto out; > + } > + > + ret = get_io_sizes(fd); > + if (ret) > + goto out; > + > + ret = get_flags(fd); > + if (ret) > + goto out; > + > + for (i = 0; i < numtests; ++i) { > + ret = do_truncate(fd, 0); > + if (ret) > + goto out; > + run_test(fd, &seek_tests[i]); > + } > + > +out: > + if (fd > -1) > + close(fd); > + return ret; > +} > -- > 1.7.5.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Cheers, Andreas ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-27 18:32 ` Andreas Dilger @ 2011-06-27 18:47 ` Josef Bacik 0 siblings, 0 replies; 74+ messages in thread From: Josef Bacik @ 2011-06-27 18:47 UTC (permalink / raw) To: Andreas Dilger; +Cc: linux-fsdevel, linux-btrfs, linux-kernel, viro On 06/27/2011 02:32 PM, Andreas Dilger wrote: > On 2011-06-27, at 12:02 PM, Josef Bacik wrote: <snip> >> + >> +#define SEEK_DATA 3 >> +#define SEEK_HOLE 4 > > These should probably be "#ifndef SEEK_DATA" so that gcc doesn't complain > in the future when these are added to a standard header. > Good point, I will fix that, thanks. <snip> >> + >> + pos = lseek(fd, 0, SEEK_HOLE); >> + if (pos == alloc_size * 2) { >> + if (!(flags & QUIET)) >> + printf("File system does not recognize holes, the only " >> + "hole found will be at the end.\n"); >> + flags |= FS_NO_HOLES; > > This is a question that I've also had about compatibility with older > (well, every) Linux kernel that does not support SEEK_{HOLE,DATA} > today. > If you look at the xfstest part of it, I grep for a complaint about not support SEEK_HOLE and I just say _notrun. I ran this test on an old kernel and it just skipped the test. > My reading of the existing generic_file_llseek() and default_llseek() > code, along with most filesystem-specific llseek() implementations is > that they will happily ignore the @whence parameter if it is not > known, and pretend like it is 0 (SEEK_SET), so they will just set the > position to the @offset parameter and return this value. In that > case, the above "SEEK_HOLE" test would incorrectly fail on every > Linux kernel in existence today because the returned pos == 0. > First, older kernels will check for whence > SEEK_MAX and automatically return -EINVAL, so we are ok there. Second, I looked at everybody in fs/ and changed anybody that did what you suggest. Anybody that I didn't change will return -EINVAL properly so I didn't touch them. I also looked at the drivers that didn't use default_llseek/seq_lseek or whatever and they all seem to handle things properly, though I'm sure I missed somebody. > Should applications call both SEEK_HOLE and SEEK_DATA with @offset=0, > and if they return the same values (which is normally impossible, > decide that the kernel does not support this SEEK_* functionality? Yeah if you want to be super careful. I mean for all file systems we should be ok with my patches, but if you hit some weird proc file that has it's llseek thing tied to something specific in the driver you may run into trouble, and by trouble you will just get weird return's for your seek. Thanks, Josef ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH 1/4] fs: add SEEK_HOLE and SEEK_DATA flags 2011-06-27 18:02 [PATCH 1/4] fs: add SEEK_HOLE and SEEK_DATA flags Josef Bacik ` (3 preceding siblings ...) 2011-06-27 18:02 ` [PATCH] xfstests 255: add a seek_data/seek_hole tester Josef Bacik @ 2011-06-27 21:04 ` Josef Bacik 4 siblings, 0 replies; 74+ messages in thread From: Josef Bacik @ 2011-06-27 21:04 UTC (permalink / raw) To: linux-fsdevel, linux-btrfs, linux-kernel, viro On 06/27/2011 02:02 PM, Josef Bacik wrote: > This just gets us ready to support the SEEK_HOLE and SEEK_DATA flags. Turns out > using fiemap in things like cp cause more problems than it solves, so lets try > and give userspace an interface that doesn't suck. We need to match solaris > here, and the definitions are > > *o* If /whence/ is SEEK_HOLE, the offset of the start of the > next hole greater than or equal to the supplied offset > is returned. The definition of a hole is provided near > the end of the DESCRIPTION. > > *o* If /whence/ is SEEK_DATA, the file pointer is set to the > start of the next non-hole file region greater than or > equal to the supplied offset. > > So in the generic case the entire file is data and there is a virtual hole at > the end. That means we will just return i_size for SEEK_HOLE and will return > the same offset for SEEK_DATA. This is how Solaris does it so we have to do it > the same way. > > Thanks, > > Signed-off-by: Josef Bacik <josef@redhat.com> > --- > fs/read_write.c | 37 +++++++++++++++++++++++++++++++++++++ > include/linux/fs.h | 4 +++- > 2 files changed, 40 insertions(+), 1 deletions(-) > > diff --git a/fs/read_write.c b/fs/read_write.c > index 5520f8a..d58551e 100644 > --- a/fs/read_write.c > +++ b/fs/read_write.c > @@ -64,6 +64,23 @@ generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin) > return file->f_pos; > offset += file->f_pos; > break; > + case SEEK_DATA: > + /* > + * In the generic case the entire file is data, so as long as > + * offset isn't at the end of the file then the offset is data. > + */ > + if (offset >= inode->i_size) > + return -ENXIO; > + break; > + case SEEK_HOLE: > + /* > + * There is a virtual hole at the end of the file, so as long as > + * offset isn't i_size or larger, return i_size. > + */ > + if (offset >= inode->i_size) > + return -ENXIO; > + offset = inode->i_size; > + break; > } > > if (offset < 0 && !unsigned_offsets(file)) > @@ -141,6 +158,26 @@ loff_t default_llseek(struct file *file, loff_t offset, int origin) > goto out; > } > offset += file->f_pos; > + break; > + case SEEK_DATA: > + /* > + * In the generic case the entire file is data, so as > + * long as offset isn't at the end of the file then the > + * offset is data. > + */ > + if (offset >= inode->i_size) > + return -ENXIO; > + break; > + case SEEK_HOLE: > + /* > + * There is a virtual hole at the end of the file, so > + * as long as offset isn't i_size or larger, return > + * i_size. > + */ > + if (offset >= inode->i_size) > + return -ENXIO; > + offset = inode->i_size; > + break; Hrm sorry I just noticed this doesn't compile. I'll fix it up and resend along with the xfstest update that Andreas pointed out. Thanks, Josef ^ permalink raw reply [flat|nested] 74+ messages in thread
* [PATCH 1/4] fs: add SEEK_HOLE and SEEK_DATA flags @ 2011-06-28 15:33 Josef Bacik 2011-06-28 15:33 ` Josef Bacik 0 siblings, 1 reply; 74+ messages in thread From: Josef Bacik @ 2011-06-28 15:33 UTC (permalink / raw) To: linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro This just gets us ready to support the SEEK_HOLE and SEEK_DATA flags. Turns out using fiemap in things like cp cause more problems than it solves, so lets try and give userspace an interface that doesn't suck. We need to match solaris here, and the definitions are *o* If /whence/ is SEEK_HOLE, the offset of the start of the next hole greater than or equal to the supplied offset is returned. The definition of a hole is provided near the end of the DESCRIPTION. *o* If /whence/ is SEEK_DATA, the file pointer is set to the start of the next non-hole file region greater than or equal to the supplied offset. So in the generic case the entire file is data and there is a virtual hole at the end. That means we will just return i_size for SEEK_HOLE and will return the same offset for SEEK_DATA. This is how Solaris does it so we have to do it the same way. Thanks, Signed-off-by: Josef Bacik <josef@redhat.com> --- fs/read_write.c | 44 +++++++++++++++++++++++++++++++++++++++++--- include/linux/fs.h | 4 +++- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/fs/read_write.c b/fs/read_write.c index 5520f8a..5907b49 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -64,6 +64,23 @@ generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin) return file->f_pos; offset += file->f_pos; break; + case SEEK_DATA: + /* + * In the generic case the entire file is data, so as long as + * offset isn't at the end of the file then the offset is data. + */ + if (offset >= inode->i_size) + return -ENXIO; + break; + case SEEK_HOLE: + /* + * There is a virtual hole at the end of the file, so as long as + * offset isn't i_size or larger, return i_size. + */ + if (offset >= inode->i_size) + return -ENXIO; + offset = inode->i_size; + break; } if (offset < 0 && !unsigned_offsets(file)) @@ -128,12 +145,13 @@ EXPORT_SYMBOL(no_llseek); loff_t default_llseek(struct file *file, loff_t offset, int origin) { + struct inode *inode = file->f_path.dentry->d_inode; loff_t retval; - mutex_lock(&file->f_dentry->d_inode->i_mutex); + mutex_lock(&inode->i_mutex); switch (origin) { case SEEK_END: - offset += i_size_read(file->f_path.dentry->d_inode); + offset += i_size_read(inode); break; case SEEK_CUR: if (offset == 0) { @@ -141,6 +159,26 @@ loff_t default_llseek(struct file *file, loff_t offset, int origin) goto out; } offset += file->f_pos; + break; + case SEEK_DATA: + /* + * In the generic case the entire file is data, so as + * long as offset isn't at the end of the file then the + * offset is data. + */ + if (offset >= inode->i_size) + return -ENXIO; + break; + case SEEK_HOLE: + /* + * There is a virtual hole at the end of the file, so + * as long as offset isn't i_size or larger, return + * i_size. + */ + if (offset >= inode->i_size) + return -ENXIO; + offset = inode->i_size; + break; } retval = -EINVAL; if (offset >= 0 || unsigned_offsets(file)) { @@ -151,7 +189,7 @@ loff_t default_llseek(struct file *file, loff_t offset, int origin) retval = offset; } out: - mutex_unlock(&file->f_dentry->d_inode->i_mutex); + mutex_unlock(&inode->i_mutex); return retval; } EXPORT_SYMBOL(default_llseek); diff --git a/include/linux/fs.h b/include/linux/fs.h index b5b9792..c9156f3 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -32,7 +32,9 @@ #define SEEK_SET 0 /* seek relative to beginning of file */ #define SEEK_CUR 1 /* seek relative to current file position */ #define SEEK_END 2 /* seek relative to end of file */ -#define SEEK_MAX SEEK_END +#define SEEK_DATA 3 /* seek to the next data */ +#define SEEK_HOLE 4 /* seek to the next hole */ +#define SEEK_MAX SEEK_HOLE struct fstrim_range { __u64 start; -- 1.7.5.2 ^ permalink raw reply related [flat|nested] 74+ messages in thread
* [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-28 15:33 Josef Bacik @ 2011-06-28 15:33 ` Josef Bacik 0 siblings, 0 replies; 74+ messages in thread From: Josef Bacik @ 2011-06-28 15:33 UTC (permalink / raw) To: linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro This is a test to make sure seek_data/seek_hole is acting like it does on Solaris. It will check to see if the fs supports finding a hole or not and will adjust as necessary. Signed-off-by: Josef Bacik <josef@redhat.com> --- 255 | 71 ++++++++ 255.out | 2 + group | 1 + src/Makefile | 2 +- src/seek-tester.c | 475 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 550 insertions(+), 1 deletions(-) create mode 100755 255 create mode 100644 255.out create mode 100644 src/seek-tester.c diff --git a/255 b/255 new file mode 100755 index 0000000..4bb4d0b --- /dev/null +++ b/255 @@ -0,0 +1,71 @@ +#! /bin/bash +# FS QA Test No. 255 +# +# Test SEEK_DATA and SEEK_HOLE +# +#----------------------------------------------------------------------- +# Copyright (c) 2011 Red Hat. All Rights Reserved. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +# +#----------------------------------------------------------------------- +# +# creator +owner=josef@redhat.com + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! + +_cleanup() +{ + rm -f $tmp.* +} + +trap "_cleanup ; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +# real QA test starts here +_supported_fs generic +_supported_os Linux + +testfile=$TEST_DIR/seek_test.$$ +logfile=$TEST_DIR/seek_test.$$.log + +[ -x $here/src/seek-tester ] || _notrun "seek-tester not built" + +_cleanup() +{ + rm -f $testfile + rm -f $logfile +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +echo "Silence is golden" +$here/src/seek-tester -q $testfile 2>&1 | tee -a $logfile + +if grep -q "SEEK_HOLE is not supported" $logfile; then + _notrun "SEEK_HOLE/SEEK_DATA not supported by this kernel" +fi + +rm -f $logfile +rm -f $testfile + +status=0 ; exit diff --git a/255.out b/255.out new file mode 100644 index 0000000..7eefb82 --- /dev/null +++ b/255.out @@ -0,0 +1,2 @@ +QA output created by 255 +Silence is golden diff --git a/group b/group index 1f86075..c045e70 100644 --- a/group +++ b/group @@ -368,3 +368,4 @@ deprecated 252 auto quick prealloc 253 auto quick 254 auto quick +255 auto quick diff --git a/src/Makefile b/src/Makefile index 91088bf..ccdaeec 100644 --- a/src/Makefile +++ b/src/Makefile @@ -17,7 +17,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \ locktest unwritten_mmap bulkstat_unlink_test t_stripealign \ bulkstat_unlink_test_modified t_dir_offset t_futimens t_immutable \ - stale_handle pwrite_mmap_blocked fstrim + stale_handle pwrite_mmap_blocked fstrim seek-tester SUBDIRS = diff --git a/src/seek-tester.c b/src/seek-tester.c new file mode 100644 index 0000000..5141b45 --- /dev/null +++ b/src/seek-tester.c @@ -0,0 +1,475 @@ +/* + * Copyright (C) 2011 Oracle. All rights reserved. + * Copyright (C) 2011 Red Hat. All rights reserved. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public + * License v2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public + * License along with this program; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place - Suite 330, + * Boston, MA 021110-1307, USA. + */ + +#define _XOPEN_SOURCE 500 +#include <sys/types.h> +#include <sys/stat.h> +#include <errno.h> +#include <fcntl.h> +#include <stdio.h> +#include <string.h> +#include <unistd.h> +#include <stdlib.h> + +#ifndef SEEK_DATA +#define SEEK_DATA 3 +#define SEEK_HOLE 4 +#endif + +#define FS_NO_HOLES (1 << 0) +#define QUIET (1 << 1) + +static blksize_t alloc_size; +static unsigned flags = 0; + +static int get_io_sizes(int fd) +{ + struct stat buf; + int ret; + + ret = fstat(fd, &buf); + if (ret) + fprintf(stderr, " ERROR %d: Failed to find io blocksize\n", + errno); + + /* st_blksize is typically also the allocation size */ + alloc_size = buf.st_blksize; + + if (!(flags & QUIET)) + printf("Allocation size: %ld\n", alloc_size); + + return ret; +} + +#define do_free(x) do { if(x) free(x); } while(0); + +static void *do_malloc(size_t size) +{ + void *buf; + + buf = malloc(size); + if (!buf) + fprintf(stderr, " ERROR: Unable to allocate %ld bytes\n", + (long)size); + + return buf; +} + +static int do_truncate(int fd, off_t length) +{ + int ret; + + ret = ftruncate(fd, length); + if (ret) + fprintf(stderr, " ERROR %d: Failed to extend file " + "to %ld bytes\n", errno, (long)length); + return ret; +} + +static ssize_t do_pwrite(int fd, const void *buf, size_t count, off_t offset) +{ + ssize_t ret, written = 0; + + while (count > written) { + ret = pwrite(fd, buf + written, count - written, offset + written); + if (ret < 0) { + fprintf(stderr, " ERROR %d: Failed to write %ld " + "bytes\n", errno, (long)count); + return ret; + } + written += ret; + } + + return 0; +} + +static int do_lseek(int testnum, int subtest, int fd, int origin, off_t set, + off_t exp) +{ + off_t pos; + int ret = -1; + + pos = lseek(fd, set, origin); + + if (pos != exp) { + fprintf(stderr, " ERROR in Test %d.%d: POS expected %ld, " + "got %ld\n", testnum, subtest, (long)exp, (long)pos); + goto out; + } + + if (pos == -1 && errno != ENXIO) { + fprintf(stderr, " ERROR in Test %d.%d: ERRNO expected %d, " + "got %d\n", testnum, subtest, ENXIO, errno); + goto out; + } + + ret = 0; + +out: + return ret; +} + +static int get_flags(int fd) +{ + const char *buf = "ABCDEF"; + ssize_t written; + off_t pos; + int ret; + + ret = do_truncate(fd, alloc_size * 2); + if (ret) + return ret; + + written = do_pwrite(fd, buf, strlen(buf), 0); + if (written) + return -1; + + pos = lseek(fd, 0, SEEK_HOLE); + if (pos == alloc_size * 2) { + if (!(flags & QUIET)) + printf("File system does not recognize holes, the only " + "hole found will be at the end.\n"); + flags |= FS_NO_HOLES; + } else if (pos == (off_t)-1) { + fprintf(stderr, "SEEK_HOLE is not supported\n"); + return -1; + } + + return 0; +} + +/* test hole data hole data */ +static int test06(int fd, int testnum) +{ + int ret = 0; + char *buf = NULL; + int bufsz = alloc_size; + int filsz = bufsz * 4; + int off; + + if (flags & FS_NO_HOLES) + return 1; + + /* HOLE - DATA - HOLE - DATA */ + /* Each unit is bufsz */ + + buf = do_malloc(bufsz); + if (!buf) + goto out; + memset(buf, 'a', bufsz); + + ret = do_pwrite(fd, buf, bufsz, bufsz); + if (!ret) + do_pwrite(fd, buf, bufsz, bufsz * 3); + if (ret) + goto out; + + /* offset at the beginning */ + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, 0); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, 1); + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, bufsz); + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, bufsz); + + /* offset around first hole-data boundary */ + off = bufsz; + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, off - 1, off - 1); + ret += do_lseek(testnum, 6, fd, SEEK_DATA, off - 1, off); + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, off, bufsz * 2); + ret += do_lseek(testnum, 8, fd, SEEK_DATA, off, off); + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, off + 1, bufsz * 2); + ret += do_lseek(testnum, 10, fd, SEEK_DATA, off + 1, off + 1); + + /* offset around data-hole boundary */ + off = bufsz * 2; + ret += do_lseek(testnum, 11, fd, SEEK_HOLE, off - 1, off); + ret += do_lseek(testnum, 12, fd, SEEK_DATA, off - 1, off - 1); + ret += do_lseek(testnum, 13, fd, SEEK_HOLE, off, off); + ret += do_lseek(testnum, 14, fd, SEEK_DATA, off, bufsz * 3); + ret += do_lseek(testnum, 15, fd, SEEK_HOLE, off + 1, off + 1); + ret += do_lseek(testnum, 16, fd, SEEK_DATA, off + 1, bufsz * 3); + + /* offset around second hole-data boundary */ + off = bufsz * 3; + ret += do_lseek(testnum, 17, fd, SEEK_HOLE, off - 1, off - 1); + ret += do_lseek(testnum, 18, fd, SEEK_DATA, off - 1, off); + ret += do_lseek(testnum, 19, fd, SEEK_HOLE, off, filsz); + ret += do_lseek(testnum, 20, fd, SEEK_DATA, off, off); + ret += do_lseek(testnum, 21, fd, SEEK_HOLE, off + 1, filsz); + ret += do_lseek(testnum, 22, fd, SEEK_DATA, off + 1, off + 1); + + /* offset around the end of file */ + off = filsz; + ret += do_lseek(testnum, 23, fd, SEEK_HOLE, off - 1, filsz); + ret += do_lseek(testnum, 24, fd, SEEK_DATA, off - 1, filsz - 1); + ret += do_lseek(testnum, 25, fd, SEEK_HOLE, off, -1); + ret += do_lseek(testnum, 26, fd, SEEK_DATA, off, -1); + ret += do_lseek(testnum, 27, fd, SEEK_HOLE, off + 1, -1); + ret += do_lseek(testnum, 28, fd, SEEK_DATA, off + 1, -1); + +out: + do_free(buf); + return ret; +} + +/* test file with data at the beginning and a hole at the end */ +static int test05(int fd, int testnum) +{ + int ret = -1; + char *buf = NULL; + int bufsz = alloc_size; + int filsz = bufsz * 4; + + if (flags & FS_NO_HOLES) + return 1; + + /* DATA - HOLE */ + /* Each unit is bufsz */ + + buf = do_malloc(bufsz); + if (!buf) + goto out; + memset(buf, 'a', bufsz); + + ret = do_truncate(fd, filsz); + if (!ret) + ret = do_pwrite(fd, buf, bufsz, 0); + if (ret) + goto out; + + /* offset at the beginning */ + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, bufsz); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, bufsz); + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, 0); + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, 1); + + /* offset around data-hole boundary */ + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, bufsz - 1, bufsz); + ret += do_lseek(testnum, 6, fd, SEEK_DATA, bufsz - 1, bufsz - 1); + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, bufsz, bufsz); + ret += do_lseek(testnum, 8, fd, SEEK_DATA, bufsz, -1); + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, bufsz + 1, bufsz + 1); + ret += do_lseek(testnum, 10, fd, SEEK_DATA, bufsz + 1, -1); + + /* offset around eof */ + ret += do_lseek(testnum, 11, fd, SEEK_HOLE, filsz - 1, filsz - 1); + ret += do_lseek(testnum, 12, fd, SEEK_DATA, filsz - 1, -1); + ret += do_lseek(testnum, 13, fd, SEEK_HOLE, filsz, -1); + ret += do_lseek(testnum, 14, fd, SEEK_DATA, filsz, -1); + ret += do_lseek(testnum, 15, fd, SEEK_HOLE, filsz + 1, -1); + ret += do_lseek(testnum, 16, fd, SEEK_DATA, filsz + 1, -1); + +out: + do_free(buf); + return ret; +} + +/* test hole begin and data end */ +static int test04(int fd, int testnum) +{ + int ret; + char *buf = "ABCDEFGH"; + int bufsz = sizeof(buf); + int holsz = alloc_size * 2; + int filsz = holsz + bufsz; + + if (flags & FS_NO_HOLES) + return 1; + + /* HOLE - DATA */ + + ret = do_pwrite(fd, buf, bufsz, holsz); + if (ret) + goto out; + + /* offset at the beginning */ + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, 0); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, 1); + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, holsz); + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, holsz); + + /* offset around hole-data boundary */ + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, holsz - 1, holsz - 1); + ret += do_lseek(testnum, 6, fd, SEEK_DATA, holsz - 1, holsz); + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, holsz, filsz); + ret += do_lseek(testnum, 8, fd, SEEK_DATA, holsz, holsz); + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, holsz + 1, filsz); + ret += do_lseek(testnum, 10, fd, SEEK_DATA, holsz + 1, holsz + 1); + + /* offset around eof */ + ret += do_lseek(testnum, 11, fd, SEEK_HOLE, filsz - 1, filsz); + ret += do_lseek(testnum, 12, fd, SEEK_DATA, filsz - 1, filsz - 1); + ret += do_lseek(testnum, 13, fd, SEEK_HOLE, filsz, -1); + ret += do_lseek(testnum, 14, fd, SEEK_DATA, filsz, -1); + ret += do_lseek(testnum, 15, fd, SEEK_HOLE, filsz + 1, -1); + ret += do_lseek(testnum, 16, fd, SEEK_DATA, filsz + 1, -1); +out: + return ret; +} + +/* test full file */ +static int test03(int fd, int testnum) +{ + char *buf = NULL; + int bufsz = alloc_size + 100; + int ret = -1; + + buf = do_malloc(bufsz); + if (!buf) + goto out; + memset(buf, 'a', bufsz); + + ret = do_pwrite(fd, buf, bufsz, 0); + if (ret) + goto out; + + /* offset at the beginning */ + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, bufsz); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, bufsz); + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, 0); + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, 1); + + /* offset around eof */ + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, bufsz - 1, bufsz); + ret += do_lseek(testnum, 6, fd, SEEK_DATA, bufsz - 1, bufsz - 1); + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, bufsz, -1); + ret += do_lseek(testnum, 8, fd, SEEK_DATA, bufsz, -1); + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, bufsz + 1, -1); + ret += do_lseek(testnum, 10, fd, SEEK_DATA, bufsz + 1, -1); + +out: + do_free(buf); + return ret; +} + +/* test empty file */ +static int test02(int fd, int testnum) +{ + int ret = 0; + + ret += do_lseek(testnum, 1, fd, SEEK_DATA, 0, -1); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 0, -1); + ret += do_lseek(testnum, 3, fd, SEEK_HOLE, 1, -1); + + return ret; +} + +/* test feature support */ +static int test01(int fd, int testnum) +{ + int ret; + char buf[] = "ABCDEFGH"; + int bufsz = sizeof(buf); + + ret = do_pwrite(fd, buf, bufsz, 0); + if (ret) + goto out; + + ret += do_lseek(testnum, 1, fd, SEEK_DATA, 0, 0); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 0, bufsz); + +out: + return ret; +} + +struct testrec { + int test_num; + int (*test_func)(int fd, int testnum); + char *test_desc; +}; + +struct testrec seek_tests[] = { + { 1, test01, "Test basic support" }, + { 2, test02, "Test an empty file" }, + { 3, test03, "Test a full file" }, + { 4, test04, "Test file hole at beg, data at end" }, + { 5, test05, "Test file data at beg, hole at end" }, + { 6, test06, "Test file hole data hole data" }, +}; + +static int run_test(int fd, struct testrec *tr) +{ + int ret; + + ret = tr->test_func(fd, tr->test_num); + if (!(flags & QUIET)) + printf("%02d. %-50s\t%s\n", tr->test_num, tr->test_desc, + ret < 0 ? "FAIL" : (ret == 0 ? "SUCC" : "NOT RUN")); + return ret; +} + +void print_help() +{ + printf("seek-test [-h] [-q] filename\n"); + printf("\t-h - this message\n"); + printf("\t-q - quiet, no output\n"); + printf("\tfilename - file to use for the test\n"); +} + +int main(int argc, char **argv) +{ + int ret = -1; + int i, fd = -1; + int c; + int numtests = sizeof(seek_tests) / sizeof(struct testrec); + + while ((c = getopt(argc, argv, "qh")) != -1) { + switch (c) { + case 'q': + flags |= QUIET; + break; + case 'h': + print_help(); + exit(0); + default: + print_help(); + exit(1); + } + } + + if (optind >= argc) { + print_help(); + exit(1); + } + + fd = open(argv[optind], O_RDWR|O_CREAT|O_TRUNC, 0644); + if (fd < 0) { + fprintf(stderr, "Failed to open testfile: %d\n", errno); + goto out; + } + + ret = get_io_sizes(fd); + if (ret) + goto out; + + ret = get_flags(fd); + if (ret) + goto out; + + for (i = 0; i < numtests; ++i) { + ret = do_truncate(fd, 0); + if (ret) + goto out; + run_test(fd, &seek_tests[i]); + } + +out: + if (fd > -1) + close(fd); + return ret; +} -- 1.7.5.2 ^ permalink raw reply related [flat|nested] 74+ messages in thread
* [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-28 15:33 ` Josef Bacik 0 siblings, 0 replies; 74+ messages in thread From: Josef Bacik @ 2011-06-28 15:33 UTC (permalink / raw) To: linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro This is a test to make sure seek_data/seek_hole is acting like it does on Solaris. It will check to see if the fs supports finding a hole or not and will adjust as necessary. Signed-off-by: Josef Bacik <josef@redhat.com> --- 255 | 71 ++++++++ 255.out | 2 + group | 1 + src/Makefile | 2 +- src/seek-tester.c | 475 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 550 insertions(+), 1 deletions(-) create mode 100755 255 create mode 100644 255.out create mode 100644 src/seek-tester.c diff --git a/255 b/255 new file mode 100755 index 0000000..4bb4d0b --- /dev/null +++ b/255 @@ -0,0 +1,71 @@ +#! /bin/bash +# FS QA Test No. 255 +# +# Test SEEK_DATA and SEEK_HOLE +# +#----------------------------------------------------------------------- +# Copyright (c) 2011 Red Hat. All Rights Reserved. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +# +#----------------------------------------------------------------------- +# +# creator +owner=josef@redhat.com + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! + +_cleanup() +{ + rm -f $tmp.* +} + +trap "_cleanup ; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +# real QA test starts here +_supported_fs generic +_supported_os Linux + +testfile=$TEST_DIR/seek_test.$$ +logfile=$TEST_DIR/seek_test.$$.log + +[ -x $here/src/seek-tester ] || _notrun "seek-tester not built" + +_cleanup() +{ + rm -f $testfile + rm -f $logfile +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +echo "Silence is golden" +$here/src/seek-tester -q $testfile 2>&1 | tee -a $logfile + +if grep -q "SEEK_HOLE is not supported" $logfile; then + _notrun "SEEK_HOLE/SEEK_DATA not supported by this kernel" +fi + +rm -f $logfile +rm -f $testfile + +status=0 ; exit diff --git a/255.out b/255.out new file mode 100644 index 0000000..7eefb82 --- /dev/null +++ b/255.out @@ -0,0 +1,2 @@ +QA output created by 255 +Silence is golden diff --git a/group b/group index 1f86075..c045e70 100644 --- a/group +++ b/group @@ -368,3 +368,4 @@ deprecated 252 auto quick prealloc 253 auto quick 254 auto quick +255 auto quick diff --git a/src/Makefile b/src/Makefile index 91088bf..ccdaeec 100644 --- a/src/Makefile +++ b/src/Makefile @@ -17,7 +17,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \ locktest unwritten_mmap bulkstat_unlink_test t_stripealign \ bulkstat_unlink_test_modified t_dir_offset t_futimens t_immutable \ - stale_handle pwrite_mmap_blocked fstrim + stale_handle pwrite_mmap_blocked fstrim seek-tester SUBDIRS = diff --git a/src/seek-tester.c b/src/seek-tester.c new file mode 100644 index 0000000..5141b45 --- /dev/null +++ b/src/seek-tester.c @@ -0,0 +1,475 @@ +/* + * Copyright (C) 2011 Oracle. All rights reserved. + * Copyright (C) 2011 Red Hat. All rights reserved. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public + * License v2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public + * License along with this program; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place - Suite 330, + * Boston, MA 021110-1307, USA. + */ + +#define _XOPEN_SOURCE 500 +#include <sys/types.h> +#include <sys/stat.h> +#include <errno.h> +#include <fcntl.h> +#include <stdio.h> +#include <string.h> +#include <unistd.h> +#include <stdlib.h> + +#ifndef SEEK_DATA +#define SEEK_DATA 3 +#define SEEK_HOLE 4 +#endif + +#define FS_NO_HOLES (1 << 0) +#define QUIET (1 << 1) + +static blksize_t alloc_size; +static unsigned flags = 0; + +static int get_io_sizes(int fd) +{ + struct stat buf; + int ret; + + ret = fstat(fd, &buf); + if (ret) + fprintf(stderr, " ERROR %d: Failed to find io blocksize\n", + errno); + + /* st_blksize is typically also the allocation size */ + alloc_size = buf.st_blksize; + + if (!(flags & QUIET)) + printf("Allocation size: %ld\n", alloc_size); + + return ret; +} + +#define do_free(x) do { if(x) free(x); } while(0); + +static void *do_malloc(size_t size) +{ + void *buf; + + buf = malloc(size); + if (!buf) + fprintf(stderr, " ERROR: Unable to allocate %ld bytes\n", + (long)size); + + return buf; +} + +static int do_truncate(int fd, off_t length) +{ + int ret; + + ret = ftruncate(fd, length); + if (ret) + fprintf(stderr, " ERROR %d: Failed to extend file " + "to %ld bytes\n", errno, (long)length); + return ret; +} + +static ssize_t do_pwrite(int fd, const void *buf, size_t count, off_t offset) +{ + ssize_t ret, written = 0; + + while (count > written) { + ret = pwrite(fd, buf + written, count - written, offset + written); + if (ret < 0) { + fprintf(stderr, " ERROR %d: Failed to write %ld " + "bytes\n", errno, (long)count); + return ret; + } + written += ret; + } + + return 0; +} + +static int do_lseek(int testnum, int subtest, int fd, int origin, off_t set, + off_t exp) +{ + off_t pos; + int ret = -1; + + pos = lseek(fd, set, origin); + + if (pos != exp) { + fprintf(stderr, " ERROR in Test %d.%d: POS expected %ld, " + "got %ld\n", testnum, subtest, (long)exp, (long)pos); + goto out; + } + + if (pos == -1 && errno != ENXIO) { + fprintf(stderr, " ERROR in Test %d.%d: ERRNO expected %d, " + "got %d\n", testnum, subtest, ENXIO, errno); + goto out; + } + + ret = 0; + +out: + return ret; +} + +static int get_flags(int fd) +{ + const char *buf = "ABCDEF"; + ssize_t written; + off_t pos; + int ret; + + ret = do_truncate(fd, alloc_size * 2); + if (ret) + return ret; + + written = do_pwrite(fd, buf, strlen(buf), 0); + if (written) + return -1; + + pos = lseek(fd, 0, SEEK_HOLE); + if (pos == alloc_size * 2) { + if (!(flags & QUIET)) + printf("File system does not recognize holes, the only " + "hole found will be at the end.\n"); + flags |= FS_NO_HOLES; + } else if (pos == (off_t)-1) { + fprintf(stderr, "SEEK_HOLE is not supported\n"); + return -1; + } + + return 0; +} + +/* test hole data hole data */ +static int test06(int fd, int testnum) +{ + int ret = 0; + char *buf = NULL; + int bufsz = alloc_size; + int filsz = bufsz * 4; + int off; + + if (flags & FS_NO_HOLES) + return 1; + + /* HOLE - DATA - HOLE - DATA */ + /* Each unit is bufsz */ + + buf = do_malloc(bufsz); + if (!buf) + goto out; + memset(buf, 'a', bufsz); + + ret = do_pwrite(fd, buf, bufsz, bufsz); + if (!ret) + do_pwrite(fd, buf, bufsz, bufsz * 3); + if (ret) + goto out; + + /* offset at the beginning */ + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, 0); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, 1); + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, bufsz); + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, bufsz); + + /* offset around first hole-data boundary */ + off = bufsz; + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, off - 1, off - 1); + ret += do_lseek(testnum, 6, fd, SEEK_DATA, off - 1, off); + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, off, bufsz * 2); + ret += do_lseek(testnum, 8, fd, SEEK_DATA, off, off); + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, off + 1, bufsz * 2); + ret += do_lseek(testnum, 10, fd, SEEK_DATA, off + 1, off + 1); + + /* offset around data-hole boundary */ + off = bufsz * 2; + ret += do_lseek(testnum, 11, fd, SEEK_HOLE, off - 1, off); + ret += do_lseek(testnum, 12, fd, SEEK_DATA, off - 1, off - 1); + ret += do_lseek(testnum, 13, fd, SEEK_HOLE, off, off); + ret += do_lseek(testnum, 14, fd, SEEK_DATA, off, bufsz * 3); + ret += do_lseek(testnum, 15, fd, SEEK_HOLE, off + 1, off + 1); + ret += do_lseek(testnum, 16, fd, SEEK_DATA, off + 1, bufsz * 3); + + /* offset around second hole-data boundary */ + off = bufsz * 3; + ret += do_lseek(testnum, 17, fd, SEEK_HOLE, off - 1, off - 1); + ret += do_lseek(testnum, 18, fd, SEEK_DATA, off - 1, off); + ret += do_lseek(testnum, 19, fd, SEEK_HOLE, off, filsz); + ret += do_lseek(testnum, 20, fd, SEEK_DATA, off, off); + ret += do_lseek(testnum, 21, fd, SEEK_HOLE, off + 1, filsz); + ret += do_lseek(testnum, 22, fd, SEEK_DATA, off + 1, off + 1); + + /* offset around the end of file */ + off = filsz; + ret += do_lseek(testnum, 23, fd, SEEK_HOLE, off - 1, filsz); + ret += do_lseek(testnum, 24, fd, SEEK_DATA, off - 1, filsz - 1); + ret += do_lseek(testnum, 25, fd, SEEK_HOLE, off, -1); + ret += do_lseek(testnum, 26, fd, SEEK_DATA, off, -1); + ret += do_lseek(testnum, 27, fd, SEEK_HOLE, off + 1, -1); + ret += do_lseek(testnum, 28, fd, SEEK_DATA, off + 1, -1); + +out: + do_free(buf); + return ret; +} + +/* test file with data at the beginning and a hole at the end */ +static int test05(int fd, int testnum) +{ + int ret = -1; + char *buf = NULL; + int bufsz = alloc_size; + int filsz = bufsz * 4; + + if (flags & FS_NO_HOLES) + return 1; + + /* DATA - HOLE */ + /* Each unit is bufsz */ + + buf = do_malloc(bufsz); + if (!buf) + goto out; + memset(buf, 'a', bufsz); + + ret = do_truncate(fd, filsz); + if (!ret) + ret = do_pwrite(fd, buf, bufsz, 0); + if (ret) + goto out; + + /* offset at the beginning */ + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, bufsz); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, bufsz); + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, 0); + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, 1); + + /* offset around data-hole boundary */ + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, bufsz - 1, bufsz); + ret += do_lseek(testnum, 6, fd, SEEK_DATA, bufsz - 1, bufsz - 1); + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, bufsz, bufsz); + ret += do_lseek(testnum, 8, fd, SEEK_DATA, bufsz, -1); + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, bufsz + 1, bufsz + 1); + ret += do_lseek(testnum, 10, fd, SEEK_DATA, bufsz + 1, -1); + + /* offset around eof */ + ret += do_lseek(testnum, 11, fd, SEEK_HOLE, filsz - 1, filsz - 1); + ret += do_lseek(testnum, 12, fd, SEEK_DATA, filsz - 1, -1); + ret += do_lseek(testnum, 13, fd, SEEK_HOLE, filsz, -1); + ret += do_lseek(testnum, 14, fd, SEEK_DATA, filsz, -1); + ret += do_lseek(testnum, 15, fd, SEEK_HOLE, filsz + 1, -1); + ret += do_lseek(testnum, 16, fd, SEEK_DATA, filsz + 1, -1); + +out: + do_free(buf); + return ret; +} + +/* test hole begin and data end */ +static int test04(int fd, int testnum) +{ + int ret; + char *buf = "ABCDEFGH"; + int bufsz = sizeof(buf); + int holsz = alloc_size * 2; + int filsz = holsz + bufsz; + + if (flags & FS_NO_HOLES) + return 1; + + /* HOLE - DATA */ + + ret = do_pwrite(fd, buf, bufsz, holsz); + if (ret) + goto out; + + /* offset at the beginning */ + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, 0); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, 1); + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, holsz); + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, holsz); + + /* offset around hole-data boundary */ + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, holsz - 1, holsz - 1); + ret += do_lseek(testnum, 6, fd, SEEK_DATA, holsz - 1, holsz); + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, holsz, filsz); + ret += do_lseek(testnum, 8, fd, SEEK_DATA, holsz, holsz); + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, holsz + 1, filsz); + ret += do_lseek(testnum, 10, fd, SEEK_DATA, holsz + 1, holsz + 1); + + /* offset around eof */ + ret += do_lseek(testnum, 11, fd, SEEK_HOLE, filsz - 1, filsz); + ret += do_lseek(testnum, 12, fd, SEEK_DATA, filsz - 1, filsz - 1); + ret += do_lseek(testnum, 13, fd, SEEK_HOLE, filsz, -1); + ret += do_lseek(testnum, 14, fd, SEEK_DATA, filsz, -1); + ret += do_lseek(testnum, 15, fd, SEEK_HOLE, filsz + 1, -1); + ret += do_lseek(testnum, 16, fd, SEEK_DATA, filsz + 1, -1); +out: + return ret; +} + +/* test full file */ +static int test03(int fd, int testnum) +{ + char *buf = NULL; + int bufsz = alloc_size + 100; + int ret = -1; + + buf = do_malloc(bufsz); + if (!buf) + goto out; + memset(buf, 'a', bufsz); + + ret = do_pwrite(fd, buf, bufsz, 0); + if (ret) + goto out; + + /* offset at the beginning */ + ret += do_lseek(testnum, 1, fd, SEEK_HOLE, 0, bufsz); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 1, bufsz); + ret += do_lseek(testnum, 3, fd, SEEK_DATA, 0, 0); + ret += do_lseek(testnum, 4, fd, SEEK_DATA, 1, 1); + + /* offset around eof */ + ret += do_lseek(testnum, 5, fd, SEEK_HOLE, bufsz - 1, bufsz); + ret += do_lseek(testnum, 6, fd, SEEK_DATA, bufsz - 1, bufsz - 1); + ret += do_lseek(testnum, 7, fd, SEEK_HOLE, bufsz, -1); + ret += do_lseek(testnum, 8, fd, SEEK_DATA, bufsz, -1); + ret += do_lseek(testnum, 9, fd, SEEK_HOLE, bufsz + 1, -1); + ret += do_lseek(testnum, 10, fd, SEEK_DATA, bufsz + 1, -1); + +out: + do_free(buf); + return ret; +} + +/* test empty file */ +static int test02(int fd, int testnum) +{ + int ret = 0; + + ret += do_lseek(testnum, 1, fd, SEEK_DATA, 0, -1); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 0, -1); + ret += do_lseek(testnum, 3, fd, SEEK_HOLE, 1, -1); + + return ret; +} + +/* test feature support */ +static int test01(int fd, int testnum) +{ + int ret; + char buf[] = "ABCDEFGH"; + int bufsz = sizeof(buf); + + ret = do_pwrite(fd, buf, bufsz, 0); + if (ret) + goto out; + + ret += do_lseek(testnum, 1, fd, SEEK_DATA, 0, 0); + ret += do_lseek(testnum, 2, fd, SEEK_HOLE, 0, bufsz); + +out: + return ret; +} + +struct testrec { + int test_num; + int (*test_func)(int fd, int testnum); + char *test_desc; +}; + +struct testrec seek_tests[] = { + { 1, test01, "Test basic support" }, + { 2, test02, "Test an empty file" }, + { 3, test03, "Test a full file" }, + { 4, test04, "Test file hole at beg, data at end" }, + { 5, test05, "Test file data at beg, hole at end" }, + { 6, test06, "Test file hole data hole data" }, +}; + +static int run_test(int fd, struct testrec *tr) +{ + int ret; + + ret = tr->test_func(fd, tr->test_num); + if (!(flags & QUIET)) + printf("%02d. %-50s\t%s\n", tr->test_num, tr->test_desc, + ret < 0 ? "FAIL" : (ret == 0 ? "SUCC" : "NOT RUN")); + return ret; +} + +void print_help() +{ + printf("seek-test [-h] [-q] filename\n"); + printf("\t-h - this message\n"); + printf("\t-q - quiet, no output\n"); + printf("\tfilename - file to use for the test\n"); +} + +int main(int argc, char **argv) +{ + int ret = -1; + int i, fd = -1; + int c; + int numtests = sizeof(seek_tests) / sizeof(struct testrec); + + while ((c = getopt(argc, argv, "qh")) != -1) { + switch (c) { + case 'q': + flags |= QUIET; + break; + case 'h': + print_help(); + exit(0); + default: + print_help(); + exit(1); + } + } + + if (optind >= argc) { + print_help(); + exit(1); + } + + fd = open(argv[optind], O_RDWR|O_CREAT|O_TRUNC, 0644); + if (fd < 0) { + fprintf(stderr, "Failed to open testfile: %d\n", errno); + goto out; + } + + ret = get_io_sizes(fd); + if (ret) + goto out; + + ret = get_flags(fd); + if (ret) + goto out; + + for (i = 0; i < numtests; ++i) { + ret = do_truncate(fd, 0); + if (ret) + goto out; + run_test(fd, &seek_tests[i]); + } + +out: + if (fd > -1) + close(fd); + return ret; +} -- 1.7.5.2 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-28 15:33 ` Josef Bacik (?) (?) @ 2011-06-29 6:53 ` Dave Chinner -1 siblings, 0 replies; 74+ messages in thread From: Dave Chinner @ 2011-06-29 6:53 UTC (permalink / raw) To: Josef Bacik; +Cc: linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: > This is a test to make sure seek_data/seek_hole is acting like it doe= s on > Solaris. It will check to see if the fs supports finding a hole or n= ot and will > adjust as necessary. So I just looked at this with an eye to validating an XFS implementation, and I came up with this list of stuff that the test does not cover that I'd need to test in some way: - files with clean unwritten extents. Are they a hole or data? What's SEEK_DATA supposed to return on layout like hole-unwritten-data? i.e. needs to add fallocate to the picture... - files with dirty unwritten extents (i.e. dirty in memory, not on disk). They are most definitely data, and most filesystems will need a separate lookup path to detect dirty unwritten ranges because the state is kept separately (page cache vs extent cache). Plenty of scope for filesystem specific bugs here so needs a roubust test. - cold cache behaviour - all dirty data ranges the test creates are hot in cache and not even forced to disk, so it is not testing the no-page-cache-over-the-data-range case. i.e. it tests delalloc state tracking but not data-extent-already exists lookups during a seek. - assumes that allocation size is the block size and that holes follows block size alignment. We already know that ext4 does not follow that rule when doing small sparse writes close together in a file, and XFS is also known to fill holes when doing sparse writes past EOF. - only tests single block data extents =D1=95o doesn't cover corner cases like skipping over multiple fragmented data extents to the next hole. Some more comments in line.... > +_cleanup() > +{ > + rm -f $tmp.* > +} > + > +trap "_cleanup ; exit \$status" 0 1 2 3 15 > + > +# get standard environment, filters and checks > +. ./common.rc > +. ./common.filter > + > +# real QA test starts here > +_supported_fs generic > +_supported_os Linux > + > +testfile=3D$TEST_DIR/seek_test.$$ > +logfile=3D$TEST_DIR/seek_test.$$.log The log file is usually named $seq.full, and doesn't get placed in the filesystem being tested. It gets saved in the xfstests directory along side $seq.out.bad for analysis whenteh test fails... > +[ -x $here/src/seek-tester ] || _notrun "seek-tester not built" > + > +_cleanup() > +{ > + rm -f $testfile > + rm -f $logfile > +} > +trap "_cleanup; exit \$status" 0 1 2 3 15 > + > +echo "Silence is golden" > +$here/src/seek-tester -q $testfile 2>&1 | tee -a $logfile Personally I'd prefer the test to be a bit noisy about what it is running, especially when there are so many subtests the single invocation is running. It makes no difference to the run time ofthe test, or the output when something fails, but it at least allows you to run the test manually and see what it is doing easily... > + > +if grep -q "SEEK_HOLE is not supported" $logfile; then > + _notrun "SEEK_HOLE/SEEK_DATA not supported by this kernel" > +fi > + > +rm -f $logfile > +rm -f $testfile > + > +status=3D0 ; exit > diff --git a/255.out b/255.out > new file mode 100644 > index 0000000..7eefb82 > --- /dev/null > +++ b/255.out > @@ -0,0 +1,2 @@ > +QA output created by 255 > +Silence is golden > diff --git a/group b/group > index 1f86075..c045e70 100644 > --- a/group > +++ b/group > @@ -368,3 +368,4 @@ deprecated > 252 auto quick prealloc > 253 auto quick > 254 auto quick > +255 auto quick I'd suggest that rw and prealloc (once unwritten extent testing is added) groups should also be defined for this test. Otherwise, the test code looks ok if a bit over-engineered.... > +struct testrec { > + int test_num; > + int (*test_func)(int fd, int testnum); > + char *test_desc; > +}; > + > +struct testrec seek_tests[] =3D { > + { 1, test01, "Test basic support" }, > + { 2, test02, "Test an empty file" }, > + { 3, test03, "Test a full file" }, > + { 4, test04, "Test file hole at beg, data at end" }, > + { 5, test05, "Test file data at beg, hole at end" }, > + { 6, test06, "Test file hole data hole data" }, So, to take from the hole punch test matrix, it covers a bunch more file state transitions and cases that are just as relevant to SEEK_HOLE/SEEK_DATA. Those cases are: # 1. into a hole # 2. into allocated space # 3. into unwritten space # 4. hole -> data # 5. hole -> unwritten # 6. data -> hole # 7. data -> unwritten # 8. unwritten -> hole # 9. unwritten -> data # 10. hole -> data -> hole # 11. data -> hole -> data # 12. unwritten -> data -> unwritten # 13. data -> unwritten -> data # 14. data -> hole @ EOF # 15. data -> hole @ 0 # 16. data -> cache cold ->hole # 17. data -> hole in single block file I thikn we also need to cover most of these same cases, right? And SEEK_HOLE/SEEK data also need to explicitly separate the unwritten tests into "clean unwritten" and "dirty unwritten" and cover the transitions between regions of those states as well, right? Cheers, Dave. --=20 Dave Chinner david@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" = 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] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 6:53 ` Dave Chinner 0 siblings, 0 replies; 74+ messages in thread From: Dave Chinner @ 2011-06-29 6:53 UTC (permalink / raw) To: Josef Bacik; +Cc: linux-fsdevel, viro, linux-kernel, linux-btrfs, xfs On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: > This is a test to make sure seek_data/seek_hole is acting like it does on > Solaris. It will check to see if the fs supports finding a hole or not and will > adjust as necessary. So I just looked at this with an eye to validating an XFS implementation, and I came up with this list of stuff that the test does not cover that I'd need to test in some way: - files with clean unwritten extents. Are they a hole or data? What's SEEK_DATA supposed to return on layout like hole-unwritten-data? i.e. needs to add fallocate to the picture... - files with dirty unwritten extents (i.e. dirty in memory, not on disk). They are most definitely data, and most filesystems will need a separate lookup path to detect dirty unwritten ranges because the state is kept separately (page cache vs extent cache). Plenty of scope for filesystem specific bugs here so needs a roubust test. - cold cache behaviour - all dirty data ranges the test creates are hot in cache and not even forced to disk, so it is not testing the no-page-cache-over-the-data-range case. i.e. it tests delalloc state tracking but not data-extent-already exists lookups during a seek. - assumes that allocation size is the block size and that holes follows block size alignment. We already know that ext4 does not follow that rule when doing small sparse writes close together in a file, and XFS is also known to fill holes when doing sparse writes past EOF. - only tests single block data extents ѕo doesn't cover corner cases like skipping over multiple fragmented data extents to the next hole. Some more comments in line.... > +_cleanup() > +{ > + rm -f $tmp.* > +} > + > +trap "_cleanup ; exit \$status" 0 1 2 3 15 > + > +# get standard environment, filters and checks > +. ./common.rc > +. ./common.filter > + > +# real QA test starts here > +_supported_fs generic > +_supported_os Linux > + > +testfile=$TEST_DIR/seek_test.$$ > +logfile=$TEST_DIR/seek_test.$$.log The log file is usually named $seq.full, and doesn't get placed in the filesystem being tested. It gets saved in the xfstests directory along side $seq.out.bad for analysis whenteh test fails... > +[ -x $here/src/seek-tester ] || _notrun "seek-tester not built" > + > +_cleanup() > +{ > + rm -f $testfile > + rm -f $logfile > +} > +trap "_cleanup; exit \$status" 0 1 2 3 15 > + > +echo "Silence is golden" > +$here/src/seek-tester -q $testfile 2>&1 | tee -a $logfile Personally I'd prefer the test to be a bit noisy about what it is running, especially when there are so many subtests the single invocation is running. It makes no difference to the run time ofthe test, or the output when something fails, but it at least allows you to run the test manually and see what it is doing easily... > + > +if grep -q "SEEK_HOLE is not supported" $logfile; then > + _notrun "SEEK_HOLE/SEEK_DATA not supported by this kernel" > +fi > + > +rm -f $logfile > +rm -f $testfile > + > +status=0 ; exit > diff --git a/255.out b/255.out > new file mode 100644 > index 0000000..7eefb82 > --- /dev/null > +++ b/255.out > @@ -0,0 +1,2 @@ > +QA output created by 255 > +Silence is golden > diff --git a/group b/group > index 1f86075..c045e70 100644 > --- a/group > +++ b/group > @@ -368,3 +368,4 @@ deprecated > 252 auto quick prealloc > 253 auto quick > 254 auto quick > +255 auto quick I'd suggest that rw and prealloc (once unwritten extent testing is added) groups should also be defined for this test. Otherwise, the test code looks ok if a bit over-engineered.... > +struct testrec { > + int test_num; > + int (*test_func)(int fd, int testnum); > + char *test_desc; > +}; > + > +struct testrec seek_tests[] = { > + { 1, test01, "Test basic support" }, > + { 2, test02, "Test an empty file" }, > + { 3, test03, "Test a full file" }, > + { 4, test04, "Test file hole at beg, data at end" }, > + { 5, test05, "Test file data at beg, hole at end" }, > + { 6, test06, "Test file hole data hole data" }, So, to take from the hole punch test matrix, it covers a bunch more file state transitions and cases that are just as relevant to SEEK_HOLE/SEEK_DATA. Those cases are: # 1. into a hole # 2. into allocated space # 3. into unwritten space # 4. hole -> data # 5. hole -> unwritten # 6. data -> hole # 7. data -> unwritten # 8. unwritten -> hole # 9. unwritten -> data # 10. hole -> data -> hole # 11. data -> hole -> data # 12. unwritten -> data -> unwritten # 13. data -> unwritten -> data # 14. data -> hole @ EOF # 15. data -> hole @ 0 # 16. data -> cache cold ->hole # 17. data -> hole in single block file I thikn we also need to cover most of these same cases, right? And SEEK_HOLE/SEEK data also need to explicitly separate the unwritten tests into "clean unwritten" and "dirty unwritten" and cover the transitions between regions of those states as well, right? Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 6:53 ` Dave Chinner 0 siblings, 0 replies; 74+ messages in thread From: Dave Chinner @ 2011-06-29 6:53 UTC (permalink / raw) To: Josef Bacik; +Cc: linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: > This is a test to make sure seek_data/seek_hole is acting like it does on > Solaris. It will check to see if the fs supports finding a hole or not and will > adjust as necessary. So I just looked at this with an eye to validating an XFS implementation, and I came up with this list of stuff that the test does not cover that I'd need to test in some way: - files with clean unwritten extents. Are they a hole or data? What's SEEK_DATA supposed to return on layout like hole-unwritten-data? i.e. needs to add fallocate to the picture... - files with dirty unwritten extents (i.e. dirty in memory, not on disk). They are most definitely data, and most filesystems will need a separate lookup path to detect dirty unwritten ranges because the state is kept separately (page cache vs extent cache). Plenty of scope for filesystem specific bugs here so needs a roubust test. - cold cache behaviour - all dirty data ranges the test creates are hot in cache and not even forced to disk, so it is not testing the no-page-cache-over-the-data-range case. i.e. it tests delalloc state tracking but not data-extent-already exists lookups during a seek. - assumes that allocation size is the block size and that holes follows block size alignment. We already know that ext4 does not follow that rule when doing small sparse writes close together in a file, and XFS is also known to fill holes when doing sparse writes past EOF. - only tests single block data extents ѕo doesn't cover corner cases like skipping over multiple fragmented data extents to the next hole. Some more comments in line.... > +_cleanup() > +{ > + rm -f $tmp.* > +} > + > +trap "_cleanup ; exit \$status" 0 1 2 3 15 > + > +# get standard environment, filters and checks > +. ./common.rc > +. ./common.filter > + > +# real QA test starts here > +_supported_fs generic > +_supported_os Linux > + > +testfile=$TEST_DIR/seek_test.$$ > +logfile=$TEST_DIR/seek_test.$$.log The log file is usually named $seq.full, and doesn't get placed in the filesystem being tested. It gets saved in the xfstests directory along side $seq.out.bad for analysis whenteh test fails... > +[ -x $here/src/seek-tester ] || _notrun "seek-tester not built" > + > +_cleanup() > +{ > + rm -f $testfile > + rm -f $logfile > +} > +trap "_cleanup; exit \$status" 0 1 2 3 15 > + > +echo "Silence is golden" > +$here/src/seek-tester -q $testfile 2>&1 | tee -a $logfile Personally I'd prefer the test to be a bit noisy about what it is running, especially when there are so many subtests the single invocation is running. It makes no difference to the run time ofthe test, or the output when something fails, but it at least allows you to run the test manually and see what it is doing easily... > + > +if grep -q "SEEK_HOLE is not supported" $logfile; then > + _notrun "SEEK_HOLE/SEEK_DATA not supported by this kernel" > +fi > + > +rm -f $logfile > +rm -f $testfile > + > +status=0 ; exit > diff --git a/255.out b/255.out > new file mode 100644 > index 0000000..7eefb82 > --- /dev/null > +++ b/255.out > @@ -0,0 +1,2 @@ > +QA output created by 255 > +Silence is golden > diff --git a/group b/group > index 1f86075..c045e70 100644 > --- a/group > +++ b/group > @@ -368,3 +368,4 @@ deprecated > 252 auto quick prealloc > 253 auto quick > 254 auto quick > +255 auto quick I'd suggest that rw and prealloc (once unwritten extent testing is added) groups should also be defined for this test. Otherwise, the test code looks ok if a bit over-engineered.... > +struct testrec { > + int test_num; > + int (*test_func)(int fd, int testnum); > + char *test_desc; > +}; > + > +struct testrec seek_tests[] = { > + { 1, test01, "Test basic support" }, > + { 2, test02, "Test an empty file" }, > + { 3, test03, "Test a full file" }, > + { 4, test04, "Test file hole at beg, data at end" }, > + { 5, test05, "Test file data at beg, hole at end" }, > + { 6, test06, "Test file hole data hole data" }, So, to take from the hole punch test matrix, it covers a bunch more file state transitions and cases that are just as relevant to SEEK_HOLE/SEEK_DATA. Those cases are: # 1. into a hole # 2. into allocated space # 3. into unwritten space # 4. hole -> data # 5. hole -> unwritten # 6. data -> hole # 7. data -> unwritten # 8. unwritten -> hole # 9. unwritten -> data # 10. hole -> data -> hole # 11. data -> hole -> data # 12. unwritten -> data -> unwritten # 13. data -> unwritten -> data # 14. data -> hole @ EOF # 15. data -> hole @ 0 # 16. data -> cache cold ->hole # 17. data -> hole in single block file I thikn we also need to cover most of these same cases, right? And SEEK_HOLE/SEEK data also need to explicitly separate the unwritten tests into "clean unwritten" and "dirty unwritten" and cover the transitions between regions of those states as well, right? Cheers, Dave. -- Dave Chinner david@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 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] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 6:53 ` Dave Chinner 0 siblings, 0 replies; 74+ messages in thread From: Dave Chinner @ 2011-06-29 6:53 UTC (permalink / raw) To: Josef Bacik; +Cc: linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: > This is a test to make sure seek_data/seek_hole is acting like it does on > Solaris. It will check to see if the fs supports finding a hole or not and will > adjust as necessary. So I just looked at this with an eye to validating an XFS implementation, and I came up with this list of stuff that the test does not cover that I'd need to test in some way: - files with clean unwritten extents. Are they a hole or data? What's SEEK_DATA supposed to return on layout like hole-unwritten-data? i.e. needs to add fallocate to the picture... - files with dirty unwritten extents (i.e. dirty in memory, not on disk). They are most definitely data, and most filesystems will need a separate lookup path to detect dirty unwritten ranges because the state is kept separately (page cache vs extent cache). Plenty of scope for filesystem specific bugs here so needs a roubust test. - cold cache behaviour - all dirty data ranges the test creates are hot in cache and not even forced to disk, so it is not testing the no-page-cache-over-the-data-range case. i.e. it tests delalloc state tracking but not data-extent-already exists lookups during a seek. - assumes that allocation size is the block size and that holes follows block size alignment. We already know that ext4 does not follow that rule when doing small sparse writes close together in a file, and XFS is also known to fill holes when doing sparse writes past EOF. - only tests single block data extents ѕo doesn't cover corner cases like skipping over multiple fragmented data extents to the next hole. Some more comments in line.... > +_cleanup() > +{ > + rm -f $tmp.* > +} > + > +trap "_cleanup ; exit \$status" 0 1 2 3 15 > + > +# get standard environment, filters and checks > +. ./common.rc > +. ./common.filter > + > +# real QA test starts here > +_supported_fs generic > +_supported_os Linux > + > +testfile=$TEST_DIR/seek_test.$$ > +logfile=$TEST_DIR/seek_test.$$.log The log file is usually named $seq.full, and doesn't get placed in the filesystem being tested. It gets saved in the xfstests directory along side $seq.out.bad for analysis whenteh test fails... > +[ -x $here/src/seek-tester ] || _notrun "seek-tester not built" > + > +_cleanup() > +{ > + rm -f $testfile > + rm -f $logfile > +} > +trap "_cleanup; exit \$status" 0 1 2 3 15 > + > +echo "Silence is golden" > +$here/src/seek-tester -q $testfile 2>&1 | tee -a $logfile Personally I'd prefer the test to be a bit noisy about what it is running, especially when there are so many subtests the single invocation is running. It makes no difference to the run time ofthe test, or the output when something fails, but it at least allows you to run the test manually and see what it is doing easily... > + > +if grep -q "SEEK_HOLE is not supported" $logfile; then > + _notrun "SEEK_HOLE/SEEK_DATA not supported by this kernel" > +fi > + > +rm -f $logfile > +rm -f $testfile > + > +status=0 ; exit > diff --git a/255.out b/255.out > new file mode 100644 > index 0000000..7eefb82 > --- /dev/null > +++ b/255.out > @@ -0,0 +1,2 @@ > +QA output created by 255 > +Silence is golden > diff --git a/group b/group > index 1f86075..c045e70 100644 > --- a/group > +++ b/group > @@ -368,3 +368,4 @@ deprecated > 252 auto quick prealloc > 253 auto quick > 254 auto quick > +255 auto quick I'd suggest that rw and prealloc (once unwritten extent testing is added) groups should also be defined for this test. Otherwise, the test code looks ok if a bit over-engineered.... > +struct testrec { > + int test_num; > + int (*test_func)(int fd, int testnum); > + char *test_desc; > +}; > + > +struct testrec seek_tests[] = { > + { 1, test01, "Test basic support" }, > + { 2, test02, "Test an empty file" }, > + { 3, test03, "Test a full file" }, > + { 4, test04, "Test file hole at beg, data at end" }, > + { 5, test05, "Test file data at beg, hole at end" }, > + { 6, test06, "Test file hole data hole data" }, So, to take from the hole punch test matrix, it covers a bunch more file state transitions and cases that are just as relevant to SEEK_HOLE/SEEK_DATA. Those cases are: # 1. into a hole # 2. into allocated space # 3. into unwritten space # 4. hole -> data # 5. hole -> unwritten # 6. data -> hole # 7. data -> unwritten # 8. unwritten -> hole # 9. unwritten -> data # 10. hole -> data -> hole # 11. data -> hole -> data # 12. unwritten -> data -> unwritten # 13. data -> unwritten -> data # 14. data -> hole @ EOF # 15. data -> hole @ 0 # 16. data -> cache cold ->hole # 17. data -> hole in single block file I thikn we also need to cover most of these same cases, right? And SEEK_HOLE/SEEK data also need to explicitly separate the unwritten tests into "clean unwritten" and "dirty unwritten" and cover the transitions between regions of those states as well, right? Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-29 6:53 ` Dave Chinner @ 2011-06-29 7:40 ` Christoph Hellwig -1 siblings, 0 replies; 74+ messages in thread From: Christoph Hellwig @ 2011-06-29 7:40 UTC (permalink / raw) To: Dave Chinner Cc: Josef Bacik, linux-fsdevel, viro, linux-kernel, linux-btrfs, xfs On Wed, Jun 29, 2011 at 04:53:07PM +1000, Dave Chinner wrote: > On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: > > This is a test to make sure seek_data/seek_hole is acting like it does on > > Solaris. It will check to see if the fs supports finding a hole or not and will > > adjust as necessary. > > So I just looked at this with an eye to validating an XFS > implementation, and I came up with this list of stuff that the test > does not cover that I'd need to test in some way: > > - files with clean unwritten extents. Are they a hole or > data? What's SEEK_DATA supposed to return on layout like > hole-unwritten-data? i.e. needs to add fallocate to the > picture... > > - files with dirty unwritten extents (i.e. dirty in memory, > not on disk). They are most definitely data, and most > filesystems will need a separate lookup path to detect > dirty unwritten ranges because the state is kept > separately (page cache vs extent cache). Plenty of scope > for filesystem specific bugs here so needs a roubust test. The discussion leading up to the resurrection of SEEK_HOLE/SEEK_DATA was pretty much about that point. The conclusion based on the Sun documentation and common sense was that SEEK_DATA may only consider unwritten extents as hole if the filesystem has a way to distinguish plain unwritten extents and those that have been dirtied. Else it should be considered data. Testing for making sure dirty preallocated areas aren't wrongly reported sounds relatively easy, the rest falls into implementation details, which imho is fine. Not reporting preallocated extents as holes just is a quality of implementation issue and not a bug. ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 7:40 ` Christoph Hellwig 0 siblings, 0 replies; 74+ messages in thread From: Christoph Hellwig @ 2011-06-29 7:40 UTC (permalink / raw) To: Dave Chinner Cc: linux-kernel, Josef Bacik, xfs, viro, linux-fsdevel, linux-btrfs On Wed, Jun 29, 2011 at 04:53:07PM +1000, Dave Chinner wrote: > On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: > > This is a test to make sure seek_data/seek_hole is acting like it does on > > Solaris. It will check to see if the fs supports finding a hole or not and will > > adjust as necessary. > > So I just looked at this with an eye to validating an XFS > implementation, and I came up with this list of stuff that the test > does not cover that I'd need to test in some way: > > - files with clean unwritten extents. Are they a hole or > data? What's SEEK_DATA supposed to return on layout like > hole-unwritten-data? i.e. needs to add fallocate to the > picture... > > - files with dirty unwritten extents (i.e. dirty in memory, > not on disk). They are most definitely data, and most > filesystems will need a separate lookup path to detect > dirty unwritten ranges because the state is kept > separately (page cache vs extent cache). Plenty of scope > for filesystem specific bugs here so needs a roubust test. The discussion leading up to the resurrection of SEEK_HOLE/SEEK_DATA was pretty much about that point. The conclusion based on the Sun documentation and common sense was that SEEK_DATA may only consider unwritten extents as hole if the filesystem has a way to distinguish plain unwritten extents and those that have been dirtied. Else it should be considered data. Testing for making sure dirty preallocated areas aren't wrongly reported sounds relatively easy, the rest falls into implementation details, which imho is fine. Not reporting preallocated extents as holes just is a quality of implementation issue and not a bug. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-29 7:40 ` Christoph Hellwig (?) (?) @ 2011-06-29 10:42 ` Pádraig Brady -1 siblings, 0 replies; 74+ messages in thread From: Pádraig Brady @ 2011-06-29 10:42 UTC (permalink / raw) To: Christoph Hellwig Cc: Dave Chinner, Josef Bacik, linux-fsdevel, viro, linux-kernel, linux-btrfs, xfs On 29/06/11 08:40, Christoph Hellwig wrote: > On Wed, Jun 29, 2011 at 04:53:07PM +1000, Dave Chinner wrote: >> On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >>> This is a test to make sure seek_data/seek_hole is acting like it d= oes on >>> Solaris. It will check to see if the fs supports finding a hole or= not and will >>> adjust as necessary. >> >> So I just looked at this with an eye to validating an XFS >> implementation, and I came up with this list of stuff that the test >> does not cover that I'd need to test in some way: >> >> - files with clean unwritten extents. Are they a hole or >> data? What's SEEK_DATA supposed to return on layout like >> hole-unwritten-data? i.e. needs to add fallocate to the >> picture... >> >> - files with dirty unwritten extents (i.e. dirty in memory, >> not on disk). They are most definitely data, and most >> filesystems will need a separate lookup path to detect >> dirty unwritten ranges because the state is kept >> separately (page cache vs extent cache). Plenty of scope >> for filesystem specific bugs here so needs a roubust test. >=20 > The discussion leading up to the resurrection of SEEK_HOLE/SEEK_DATA > was pretty much about that point. The conclusion based on the Sun > documentation and common sense was that SEEK_DATA may only consider > unwritten extents as hole if the filesystem has a way to distinguish > plain unwritten extents and those that have been dirtied. Else it > should be considered data. >=20 > Testing for making sure dirty preallocated areas aren't wrongly > reported sounds relatively easy, the rest falls into implementation > details, which imho is fine. Not reporting preallocated extents > as holes just is a quality of implementation issue and not a bug. There is the argument, that if this interface can distinguish these dirty unwritten extents, then why can't the fiemap interface too? The advantage of the fiemap interface is that it can distinguish empty extents vs holes. Empty extents will become increasingly common I think, given the fragmentation and space guarantee benefits they give= =2E It would be cool for cp for example to be able to efficiently copy empty extents from source to dest. cheers, P=E1draig. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" = 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] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 10:42 ` Pádraig Brady 0 siblings, 0 replies; 74+ messages in thread From: Pádraig Brady @ 2011-06-29 10:42 UTC (permalink / raw) To: Christoph Hellwig Cc: linux-kernel, Josef Bacik, xfs, viro, linux-fsdevel, linux-btrfs On 29/06/11 08:40, Christoph Hellwig wrote: > On Wed, Jun 29, 2011 at 04:53:07PM +1000, Dave Chinner wrote: >> On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >>> This is a test to make sure seek_data/seek_hole is acting like it does on >>> Solaris. It will check to see if the fs supports finding a hole or not and will >>> adjust as necessary. >> >> So I just looked at this with an eye to validating an XFS >> implementation, and I came up with this list of stuff that the test >> does not cover that I'd need to test in some way: >> >> - files with clean unwritten extents. Are they a hole or >> data? What's SEEK_DATA supposed to return on layout like >> hole-unwritten-data? i.e. needs to add fallocate to the >> picture... >> >> - files with dirty unwritten extents (i.e. dirty in memory, >> not on disk). They are most definitely data, and most >> filesystems will need a separate lookup path to detect >> dirty unwritten ranges because the state is kept >> separately (page cache vs extent cache). Plenty of scope >> for filesystem specific bugs here so needs a roubust test. > > The discussion leading up to the resurrection of SEEK_HOLE/SEEK_DATA > was pretty much about that point. The conclusion based on the Sun > documentation and common sense was that SEEK_DATA may only consider > unwritten extents as hole if the filesystem has a way to distinguish > plain unwritten extents and those that have been dirtied. Else it > should be considered data. > > Testing for making sure dirty preallocated areas aren't wrongly > reported sounds relatively easy, the rest falls into implementation > details, which imho is fine. Not reporting preallocated extents > as holes just is a quality of implementation issue and not a bug. There is the argument, that if this interface can distinguish these dirty unwritten extents, then why can't the fiemap interface too? The advantage of the fiemap interface is that it can distinguish empty extents vs holes. Empty extents will become increasingly common I think, given the fragmentation and space guarantee benefits they give. It would be cool for cp for example to be able to efficiently copy empty extents from source to dest. cheers, Pádraig. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 10:42 ` Pádraig Brady 0 siblings, 0 replies; 74+ messages in thread From: Pádraig Brady @ 2011-06-29 10:42 UTC (permalink / raw) To: Christoph Hellwig Cc: Dave Chinner, Josef Bacik, linux-fsdevel, viro, linux-kernel, linux-btrfs, xfs On 29/06/11 08:40, Christoph Hellwig wrote: > On Wed, Jun 29, 2011 at 04:53:07PM +1000, Dave Chinner wrote: >> On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >>> This is a test to make sure seek_data/seek_hole is acting like it does on >>> Solaris. It will check to see if the fs supports finding a hole or not and will >>> adjust as necessary. >> >> So I just looked at this with an eye to validating an XFS >> implementation, and I came up with this list of stuff that the test >> does not cover that I'd need to test in some way: >> >> - files with clean unwritten extents. Are they a hole or >> data? What's SEEK_DATA supposed to return on layout like >> hole-unwritten-data? i.e. needs to add fallocate to the >> picture... >> >> - files with dirty unwritten extents (i.e. dirty in memory, >> not on disk). They are most definitely data, and most >> filesystems will need a separate lookup path to detect >> dirty unwritten ranges because the state is kept >> separately (page cache vs extent cache). Plenty of scope >> for filesystem specific bugs here so needs a roubust test. > > The discussion leading up to the resurrection of SEEK_HOLE/SEEK_DATA > was pretty much about that point. The conclusion based on the Sun > documentation and common sense was that SEEK_DATA may only consider > unwritten extents as hole if the filesystem has a way to distinguish > plain unwritten extents and those that have been dirtied. Else it > should be considered data. > > Testing for making sure dirty preallocated areas aren't wrongly > reported sounds relatively easy, the rest falls into implementation > details, which imho is fine. Not reporting preallocated extents > as holes just is a quality of implementation issue and not a bug. There is the argument, that if this interface can distinguish these dirty unwritten extents, then why can't the fiemap interface too? The advantage of the fiemap interface is that it can distinguish empty extents vs holes. Empty extents will become increasingly common I think, given the fragmentation and space guarantee benefits they give. It would be cool for cp for example to be able to efficiently copy empty extents from source to dest. cheers, Pádraig. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 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] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 10:42 ` Pádraig Brady 0 siblings, 0 replies; 74+ messages in thread From: Pádraig Brady @ 2011-06-29 10:42 UTC (permalink / raw) To: Christoph Hellwig Cc: Dave Chinner, Josef Bacik, linux-fsdevel, viro, linux-kernel, linux-btrfs, xfs On 29/06/11 08:40, Christoph Hellwig wrote: > On Wed, Jun 29, 2011 at 04:53:07PM +1000, Dave Chinner wrote: >> On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >>> This is a test to make sure seek_data/seek_hole is acting like it does on >>> Solaris. It will check to see if the fs supports finding a hole or not and will >>> adjust as necessary. >> >> So I just looked at this with an eye to validating an XFS >> implementation, and I came up with this list of stuff that the test >> does not cover that I'd need to test in some way: >> >> - files with clean unwritten extents. Are they a hole or >> data? What's SEEK_DATA supposed to return on layout like >> hole-unwritten-data? i.e. needs to add fallocate to the >> picture... >> >> - files with dirty unwritten extents (i.e. dirty in memory, >> not on disk). They are most definitely data, and most >> filesystems will need a separate lookup path to detect >> dirty unwritten ranges because the state is kept >> separately (page cache vs extent cache). Plenty of scope >> for filesystem specific bugs here so needs a roubust test. > > The discussion leading up to the resurrection of SEEK_HOLE/SEEK_DATA > was pretty much about that point. The conclusion based on the Sun > documentation and common sense was that SEEK_DATA may only consider > unwritten extents as hole if the filesystem has a way to distinguish > plain unwritten extents and those that have been dirtied. Else it > should be considered data. > > Testing for making sure dirty preallocated areas aren't wrongly > reported sounds relatively easy, the rest falls into implementation > details, which imho is fine. Not reporting preallocated extents > as holes just is a quality of implementation issue and not a bug. There is the argument, that if this interface can distinguish these dirty unwritten extents, then why can't the fiemap interface too? The advantage of the fiemap interface is that it can distinguish empty extents vs holes. Empty extents will become increasingly common I think, given the fragmentation and space guarantee benefits they give. It would be cool for cp for example to be able to efficiently copy empty extents from source to dest. cheers, Pádraig. ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-29 10:42 ` Pádraig Brady (?) (?) @ 2011-06-29 17:29 ` Sunil Mushran -1 siblings, 0 replies; 74+ messages in thread From: Sunil Mushran @ 2011-06-29 17:29 UTC (permalink / raw) To: Pádraig Brady Cc: Christoph Hellwig, Dave Chinner, Josef Bacik, linux-fsdevel, viro, linux-kernel, linux-btrfs, xfs On 06/29/2011 03:42 AM, P=E1draig Brady wrote: > There is the argument, that if this interface can distinguish > these dirty unwritten extents, then why can't the fiemap interface to= o? > The advantage of the fiemap interface is that it can distinguish > empty extents vs holes. Empty extents will become increasingly common > I think, given the fragmentation and space guarantee benefits they gi= ve. > It would be cool for cp for example to be able to efficiently copy > empty extents from source to dest. I'm not too sure about that. Atleast not enabled by default. Most users use cp to backup data. Not empty space. In this case, this empty extent may not even be de-dupable. =46rankly I'd be happier of cp started to exploited fallocate() to crea= te larger extents before copying data into them. Atleast for the large files. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " 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] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 17:29 ` Sunil Mushran 0 siblings, 0 replies; 74+ messages in thread From: Sunil Mushran @ 2011-06-29 17:29 UTC (permalink / raw) To: Pádraig Brady Cc: linux-btrfs, linux-kernel, xfs, Christoph Hellwig, viro, linux-fsdevel, Josef Bacik On 06/29/2011 03:42 AM, Pádraig Brady wrote: > There is the argument, that if this interface can distinguish > these dirty unwritten extents, then why can't the fiemap interface too? > The advantage of the fiemap interface is that it can distinguish > empty extents vs holes. Empty extents will become increasingly common > I think, given the fragmentation and space guarantee benefits they give. > It would be cool for cp for example to be able to efficiently copy > empty extents from source to dest. I'm not too sure about that. Atleast not enabled by default. Most users use cp to backup data. Not empty space. In this case, this empty extent may not even be de-dupable. Frankly I'd be happier of cp started to exploited fallocate() to create larger extents before copying data into them. Atleast for the large files. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 17:29 ` Sunil Mushran 0 siblings, 0 replies; 74+ messages in thread From: Sunil Mushran @ 2011-06-29 17:29 UTC (permalink / raw) To: Pádraig Brady Cc: Christoph Hellwig, Dave Chinner, Josef Bacik, linux-fsdevel, viro, linux-kernel, linux-btrfs, xfs On 06/29/2011 03:42 AM, Pádraig Brady wrote: > There is the argument, that if this interface can distinguish > these dirty unwritten extents, then why can't the fiemap interface too? > The advantage of the fiemap interface is that it can distinguish > empty extents vs holes. Empty extents will become increasingly common > I think, given the fragmentation and space guarantee benefits they give. > It would be cool for cp for example to be able to efficiently copy > empty extents from source to dest. I'm not too sure about that. Atleast not enabled by default. Most users use cp to backup data. Not empty space. In this case, this empty extent may not even be de-dupable. Frankly I'd be happier of cp started to exploited fallocate() to create larger extents before copying data into them. Atleast for the large files. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" 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] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 17:29 ` Sunil Mushran 0 siblings, 0 replies; 74+ messages in thread From: Sunil Mushran @ 2011-06-29 17:29 UTC (permalink / raw) To: Pádraig Brady Cc: Christoph Hellwig, Dave Chinner, Josef Bacik, linux-fsdevel, viro, linux-kernel, linux-btrfs, xfs On 06/29/2011 03:42 AM, Pádraig Brady wrote: > There is the argument, that if this interface can distinguish > these dirty unwritten extents, then why can't the fiemap interface too? > The advantage of the fiemap interface is that it can distinguish > empty extents vs holes. Empty extents will become increasingly common > I think, given the fragmentation and space guarantee benefits they give. > It would be cool for cp for example to be able to efficiently copy > empty extents from source to dest. I'm not too sure about that. Atleast not enabled by default. Most users use cp to backup data. Not empty space. In this case, this empty extent may not even be de-dupable. Frankly I'd be happier of cp started to exploited fallocate() to create larger extents before copying data into them. Atleast for the large files. ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-29 17:29 ` Sunil Mushran @ 2011-06-29 17:36 ` Christoph Hellwig -1 siblings, 0 replies; 74+ messages in thread From: Christoph Hellwig @ 2011-06-29 17:36 UTC (permalink / raw) To: Sunil Mushran Cc: P?draig Brady, Christoph Hellwig, Dave Chinner, Josef Bacik, linux-fsdevel, viro, linux-kernel, linux-btrfs, xfs On Wed, Jun 29, 2011 at 10:29:02AM -0700, Sunil Mushran wrote: > I'm not too sure about that. Atleast not enabled by default. Most users > use cp to backup data. Not empty space. In this case, this empty extent > may not even be de-dupable. > > Frankly I'd be happier of cp started to exploited fallocate() to create larger > extents before copying data into them. Atleast for the large files. That's what delayed allocation is for. ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 17:36 ` Christoph Hellwig 0 siblings, 0 replies; 74+ messages in thread From: Christoph Hellwig @ 2011-06-29 17:36 UTC (permalink / raw) To: Sunil Mushran Cc: linux-btrfs, linux-kernel, xfs, Christoph Hellwig, viro, linux-fsdevel, P?draig Brady, Josef Bacik On Wed, Jun 29, 2011 at 10:29:02AM -0700, Sunil Mushran wrote: > I'm not too sure about that. Atleast not enabled by default. Most users > use cp to backup data. Not empty space. In this case, this empty extent > may not even be de-dupable. > > Frankly I'd be happier of cp started to exploited fallocate() to create larger > extents before copying data into them. Atleast for the large files. That's what delayed allocation is for. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-29 17:36 ` Christoph Hellwig @ 2011-06-29 17:40 ` Sunil Mushran -1 siblings, 0 replies; 74+ messages in thread From: Sunil Mushran @ 2011-06-29 17:40 UTC (permalink / raw) To: Christoph Hellwig Cc: P?draig Brady, Dave Chinner, Josef Bacik, linux-fsdevel, viro, linux-kernel, linux-btrfs, xfs On 06/29/2011 10:36 AM, Christoph Hellwig wrote: > On Wed, Jun 29, 2011 at 10:29:02AM -0700, Sunil Mushran wrote: >> I'm not too sure about that. Atleast not enabled by default. Most users >> use cp to backup data. Not empty space. In this case, this empty extent >> may not even be de-dupable. >> >> Frankly I'd be happier of cp started to exploited fallocate() to create larger >> extents before copying data into them. Atleast for the large files. > That's what delayed allocation is for. A feature fewer file systems support than fallocate(). ;) ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 17:40 ` Sunil Mushran 0 siblings, 0 replies; 74+ messages in thread From: Sunil Mushran @ 2011-06-29 17:40 UTC (permalink / raw) To: Christoph Hellwig Cc: linux-btrfs, linux-kernel, xfs, viro, linux-fsdevel, P?draig Brady, Josef Bacik On 06/29/2011 10:36 AM, Christoph Hellwig wrote: > On Wed, Jun 29, 2011 at 10:29:02AM -0700, Sunil Mushran wrote: >> I'm not too sure about that. Atleast not enabled by default. Most users >> use cp to backup data. Not empty space. In this case, this empty extent >> may not even be de-dupable. >> >> Frankly I'd be happier of cp started to exploited fallocate() to create larger >> extents before copying data into them. Atleast for the large files. > That's what delayed allocation is for. A feature fewer file systems support than fallocate(). ;) _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-29 17:29 ` Sunil Mushran (?) @ 2011-06-29 21:29 ` Pádraig Brady -1 siblings, 0 replies; 74+ messages in thread From: Pádraig Brady @ 2011-06-29 21:29 UTC (permalink / raw) To: Sunil Mushran Cc: Christoph Hellwig, Dave Chinner, Josef Bacik, linux-fsdevel, viro, linux-kernel, linux-btrfs, xfs On 29/06/11 18:29, Sunil Mushran wrote: > On 06/29/2011 03:42 AM, P=E1draig Brady wrote: >> There is the argument, that if this interface can distinguish >> these dirty unwritten extents, then why can't the fiemap interface t= oo? >> The advantage of the fiemap interface is that it can distinguish >> empty extents vs holes. Empty extents will become increasingly commo= n >> I think, given the fragmentation and space guarantee benefits they g= ive. >> It would be cool for cp for example to be able to efficiently copy >> empty extents from source to dest. >=20 > I'm not too sure about that. Atleast not enabled by default. Most use= rs > use cp to backup data. Not empty space. In this case, this empty exte= nt > may not even be de-dupable. That's a fair point. On the other hand if you wanted to start working with the backup copy, you might want it allocated to avoid fragmentation and ENOSPC issues. What we were going with was empty -> hole with cp --sparse=3Dalways and empty -> empty otherwise. If empty and hole can not be distinguished though, then this process will be impacted. >=20 > Frankly I'd be happier of cp started to exploited fallocate() to crea= te > larger > extents before copying data into them. Atleast for the large files. Yes we definitely will start doing that. That will help fragmentation and give early ENOSPC. We can't use fiemap for this at the moment (on XSF or ext4 (without a sync)) but the seek_data interface should allow us to do this to some extent (pardon the pun). cheers, P=E1draig. ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 21:29 ` Pádraig Brady 0 siblings, 0 replies; 74+ messages in thread From: Pádraig Brady @ 2011-06-29 21:29 UTC (permalink / raw) To: Sunil Mushran Cc: linux-btrfs, linux-kernel, xfs, Christoph Hellwig, viro, linux-fsdevel, Josef Bacik On 29/06/11 18:29, Sunil Mushran wrote: > On 06/29/2011 03:42 AM, Pádraig Brady wrote: >> There is the argument, that if this interface can distinguish >> these dirty unwritten extents, then why can't the fiemap interface too? >> The advantage of the fiemap interface is that it can distinguish >> empty extents vs holes. Empty extents will become increasingly common >> I think, given the fragmentation and space guarantee benefits they give. >> It would be cool for cp for example to be able to efficiently copy >> empty extents from source to dest. > > I'm not too sure about that. Atleast not enabled by default. Most users > use cp to backup data. Not empty space. In this case, this empty extent > may not even be de-dupable. That's a fair point. On the other hand if you wanted to start working with the backup copy, you might want it allocated to avoid fragmentation and ENOSPC issues. What we were going with was empty -> hole with cp --sparse=always and empty -> empty otherwise. If empty and hole can not be distinguished though, then this process will be impacted. > > Frankly I'd be happier of cp started to exploited fallocate() to create > larger > extents before copying data into them. Atleast for the large files. Yes we definitely will start doing that. That will help fragmentation and give early ENOSPC. We can't use fiemap for this at the moment (on XSF or ext4 (without a sync)) but the seek_data interface should allow us to do this to some extent (pardon the pun). cheers, Pádraig. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 21:29 ` Pádraig Brady 0 siblings, 0 replies; 74+ messages in thread From: Pádraig Brady @ 2011-06-29 21:29 UTC (permalink / raw) To: Sunil Mushran Cc: Christoph Hellwig, Dave Chinner, Josef Bacik, linux-fsdevel, viro, linux-kernel, linux-btrfs, xfs On 29/06/11 18:29, Sunil Mushran wrote: > On 06/29/2011 03:42 AM, Pádraig Brady wrote: >> There is the argument, that if this interface can distinguish >> these dirty unwritten extents, then why can't the fiemap interface too? >> The advantage of the fiemap interface is that it can distinguish >> empty extents vs holes. Empty extents will become increasingly common >> I think, given the fragmentation and space guarantee benefits they give. >> It would be cool for cp for example to be able to efficiently copy >> empty extents from source to dest. > > I'm not too sure about that. Atleast not enabled by default. Most users > use cp to backup data. Not empty space. In this case, this empty extent > may not even be de-dupable. That's a fair point. On the other hand if you wanted to start working with the backup copy, you might want it allocated to avoid fragmentation and ENOSPC issues. What we were going with was empty -> hole with cp --sparse=always and empty -> empty otherwise. If empty and hole can not be distinguished though, then this process will be impacted. > > Frankly I'd be happier of cp started to exploited fallocate() to create > larger > extents before copying data into them. Atleast for the large files. Yes we definitely will start doing that. That will help fragmentation and give early ENOSPC. We can't use fiemap for this at the moment (on XSF or ext4 (without a sync)) but the seek_data interface should allow us to do this to some extent (pardon the pun). cheers, Pádraig. ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-29 10:42 ` Pádraig Brady @ 2011-07-01 9:37 ` Christoph Hellwig -1 siblings, 0 replies; 74+ messages in thread From: Christoph Hellwig @ 2011-07-01 9:37 UTC (permalink / raw) To: P?draig Brady Cc: Christoph Hellwig, linux-kernel, Josef Bacik, xfs, viro, linux-fsdevel, linux-btrfs On Wed, Jun 29, 2011 at 11:42:38AM +0100, P?draig Brady wrote: > There is the argument, that if this interface can distinguish > these dirty unwritten extents, then why can't the fiemap interface too? > The advantage of the fiemap interface is that it can distinguish > empty extents vs holes. Empty extents will become increasingly common > I think, given the fragmentation and space guarantee benefits they give. > It would be cool for cp for example to be able to efficiently copy > empty extents from source to dest. That brings us back to square one. FIEMAP is supposed to tell you about the physical layout on disk. Unwritten extents physically always are there, but whether they might have to be copied depends entirely on in-core state. Finding that incore state in addition is not all that easy compared to simply walking the extents. People might decide it's worth for an interface like SEEK_HOLE specificly asking for that, but grafting it into FIEMAP through the backdoor is a horrible idea. ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-07-01 9:37 ` Christoph Hellwig 0 siblings, 0 replies; 74+ messages in thread From: Christoph Hellwig @ 2011-07-01 9:37 UTC (permalink / raw) To: P?draig Brady Cc: linux-btrfs, linux-kernel, xfs, Christoph Hellwig, viro, linux-fsdevel, Josef Bacik On Wed, Jun 29, 2011 at 11:42:38AM +0100, P?draig Brady wrote: > There is the argument, that if this interface can distinguish > these dirty unwritten extents, then why can't the fiemap interface too? > The advantage of the fiemap interface is that it can distinguish > empty extents vs holes. Empty extents will become increasingly common > I think, given the fragmentation and space guarantee benefits they give. > It would be cool for cp for example to be able to efficiently copy > empty extents from source to dest. That brings us back to square one. FIEMAP is supposed to tell you about the physical layout on disk. Unwritten extents physically always are there, but whether they might have to be copied depends entirely on in-core state. Finding that incore state in addition is not all that easy compared to simply walking the extents. People might decide it's worth for an interface like SEEK_HOLE specificly asking for that, but grafting it into FIEMAP through the backdoor is a horrible idea. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-29 7:40 ` Christoph Hellwig @ 2011-06-29 17:10 ` Sunil Mushran -1 siblings, 0 replies; 74+ messages in thread From: Sunil Mushran @ 2011-06-29 17:10 UTC (permalink / raw) To: Christoph Hellwig Cc: Dave Chinner, Josef Bacik, linux-fsdevel, viro, linux-kernel, linux-btrfs, xfs On 06/29/2011 12:40 AM, Christoph Hellwig wrote: > On Wed, Jun 29, 2011 at 04:53:07PM +1000, Dave Chinner wrote: >> On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >>> This is a test to make sure seek_data/seek_hole is acting like it does on >>> Solaris. It will check to see if the fs supports finding a hole or not and will >>> adjust as necessary. >> So I just looked at this with an eye to validating an XFS >> implementation, and I came up with this list of stuff that the test >> does not cover that I'd need to test in some way: >> >> - files with clean unwritten extents. Are they a hole or >> data? What's SEEK_DATA supposed to return on layout like >> hole-unwritten-data? i.e. needs to add fallocate to the >> picture... >> >> - files with dirty unwritten extents (i.e. dirty in memory, >> not on disk). They are most definitely data, and most >> filesystems will need a separate lookup path to detect >> dirty unwritten ranges because the state is kept >> separately (page cache vs extent cache). Plenty of scope >> for filesystem specific bugs here so needs a roubust test. > The discussion leading up to the resurrection of SEEK_HOLE/SEEK_DATA > was pretty much about that point. The conclusion based on the Sun > documentation and common sense was that SEEK_DATA may only consider > unwritten extents as hole if the filesystem has a way to distinguish > plain unwritten extents and those that have been dirtied. Else it > should be considered data. > > Testing for making sure dirty preallocated areas aren't wrongly > reported sounds relatively easy, the rest falls into implementation > details, which imho is fine. Not reporting preallocated extents > as holes just is a quality of implementation issue and not a bug. I agree. And if I might add my 2 cents that it would be much easier if we added another test that created files with all the worrisome boundary conditions and used SEEK_DATA/HOLE to copy the files and compared using md5sum. This would be far easier than one that expects a certain pos for each operation. ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 17:10 ` Sunil Mushran 0 siblings, 0 replies; 74+ messages in thread From: Sunil Mushran @ 2011-06-29 17:10 UTC (permalink / raw) To: Christoph Hellwig Cc: linux-kernel, Josef Bacik, xfs, viro, linux-fsdevel, linux-btrfs On 06/29/2011 12:40 AM, Christoph Hellwig wrote: > On Wed, Jun 29, 2011 at 04:53:07PM +1000, Dave Chinner wrote: >> On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >>> This is a test to make sure seek_data/seek_hole is acting like it does on >>> Solaris. It will check to see if the fs supports finding a hole or not and will >>> adjust as necessary. >> So I just looked at this with an eye to validating an XFS >> implementation, and I came up with this list of stuff that the test >> does not cover that I'd need to test in some way: >> >> - files with clean unwritten extents. Are they a hole or >> data? What's SEEK_DATA supposed to return on layout like >> hole-unwritten-data? i.e. needs to add fallocate to the >> picture... >> >> - files with dirty unwritten extents (i.e. dirty in memory, >> not on disk). They are most definitely data, and most >> filesystems will need a separate lookup path to detect >> dirty unwritten ranges because the state is kept >> separately (page cache vs extent cache). Plenty of scope >> for filesystem specific bugs here so needs a roubust test. > The discussion leading up to the resurrection of SEEK_HOLE/SEEK_DATA > was pretty much about that point. The conclusion based on the Sun > documentation and common sense was that SEEK_DATA may only consider > unwritten extents as hole if the filesystem has a way to distinguish > plain unwritten extents and those that have been dirtied. Else it > should be considered data. > > Testing for making sure dirty preallocated areas aren't wrongly > reported sounds relatively easy, the rest falls into implementation > details, which imho is fine. Not reporting preallocated extents > as holes just is a quality of implementation issue and not a bug. I agree. And if I might add my 2 cents that it would be much easier if we added another test that created files with all the worrisome boundary conditions and used SEEK_DATA/HOLE to copy the files and compared using md5sum. This would be far easier than one that expects a certain pos for each operation. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-29 17:10 ` Sunil Mushran @ 2011-06-29 17:52 ` Josef Bacik -1 siblings, 0 replies; 74+ messages in thread From: Josef Bacik @ 2011-06-29 17:52 UTC (permalink / raw) To: Sunil Mushran Cc: Christoph Hellwig, Dave Chinner, linux-fsdevel, viro, linux-kernel, linux-btrfs, xfs On 06/29/2011 01:10 PM, Sunil Mushran wrote: > On 06/29/2011 12:40 AM, Christoph Hellwig wrote: >> On Wed, Jun 29, 2011 at 04:53:07PM +1000, Dave Chinner wrote: >>> On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >>>> This is a test to make sure seek_data/seek_hole is acting like it >>>> does on >>>> Solaris. It will check to see if the fs supports finding a hole or >>>> not and will >>>> adjust as necessary. >>> So I just looked at this with an eye to validating an XFS >>> implementation, and I came up with this list of stuff that the test >>> does not cover that I'd need to test in some way: >>> >>> - files with clean unwritten extents. Are they a hole or >>> data? What's SEEK_DATA supposed to return on layout like >>> hole-unwritten-data? i.e. needs to add fallocate to the >>> picture... >>> >>> - files with dirty unwritten extents (i.e. dirty in memory, >>> not on disk). They are most definitely data, and most >>> filesystems will need a separate lookup path to detect >>> dirty unwritten ranges because the state is kept >>> separately (page cache vs extent cache). Plenty of scope >>> for filesystem specific bugs here so needs a roubust test. >> The discussion leading up to the resurrection of SEEK_HOLE/SEEK_DATA >> was pretty much about that point. The conclusion based on the Sun >> documentation and common sense was that SEEK_DATA may only consider >> unwritten extents as hole if the filesystem has a way to distinguish >> plain unwritten extents and those that have been dirtied. Else it >> should be considered data. >> >> Testing for making sure dirty preallocated areas aren't wrongly >> reported sounds relatively easy, the rest falls into implementation >> details, which imho is fine. Not reporting preallocated extents >> as holes just is a quality of implementation issue and not a bug. > > I agree. And if I might add my 2 cents that it would be much easier > if we added another test that created files with all the worrisome boundary > conditions and used SEEK_DATA/HOLE to copy the files and compared > using md5sum. This would be far easier than one that expects a certain > pos for each operation. That's a great point, I think I will rig something like that up. Thanks, Josef ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 17:52 ` Josef Bacik 0 siblings, 0 replies; 74+ messages in thread From: Josef Bacik @ 2011-06-29 17:52 UTC (permalink / raw) To: Sunil Mushran Cc: linux-kernel, xfs, Christoph Hellwig, viro, linux-fsdevel, linux-btrfs On 06/29/2011 01:10 PM, Sunil Mushran wrote: > On 06/29/2011 12:40 AM, Christoph Hellwig wrote: >> On Wed, Jun 29, 2011 at 04:53:07PM +1000, Dave Chinner wrote: >>> On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >>>> This is a test to make sure seek_data/seek_hole is acting like it >>>> does on >>>> Solaris. It will check to see if the fs supports finding a hole or >>>> not and will >>>> adjust as necessary. >>> So I just looked at this with an eye to validating an XFS >>> implementation, and I came up with this list of stuff that the test >>> does not cover that I'd need to test in some way: >>> >>> - files with clean unwritten extents. Are they a hole or >>> data? What's SEEK_DATA supposed to return on layout like >>> hole-unwritten-data? i.e. needs to add fallocate to the >>> picture... >>> >>> - files with dirty unwritten extents (i.e. dirty in memory, >>> not on disk). They are most definitely data, and most >>> filesystems will need a separate lookup path to detect >>> dirty unwritten ranges because the state is kept >>> separately (page cache vs extent cache). Plenty of scope >>> for filesystem specific bugs here so needs a roubust test. >> The discussion leading up to the resurrection of SEEK_HOLE/SEEK_DATA >> was pretty much about that point. The conclusion based on the Sun >> documentation and common sense was that SEEK_DATA may only consider >> unwritten extents as hole if the filesystem has a way to distinguish >> plain unwritten extents and those that have been dirtied. Else it >> should be considered data. >> >> Testing for making sure dirty preallocated areas aren't wrongly >> reported sounds relatively easy, the rest falls into implementation >> details, which imho is fine. Not reporting preallocated extents >> as holes just is a quality of implementation issue and not a bug. > > I agree. And if I might add my 2 cents that it would be much easier > if we added another test that created files with all the worrisome boundary > conditions and used SEEK_DATA/HOLE to copy the files and compared > using md5sum. This would be far easier than one that expects a certain > pos for each operation. That's a great point, I think I will rig something like that up. Thanks, Josef _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-29 6:53 ` Dave Chinner (?) @ 2011-06-29 13:19 ` Josef Bacik -1 siblings, 0 replies; 74+ messages in thread From: Josef Bacik @ 2011-06-29 13:19 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro On 06/29/2011 02:53 AM, Dave Chinner wrote: > On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >> This is a test to make sure seek_data/seek_hole is acting like it do= es on >> Solaris. It will check to see if the fs supports finding a hole or = not and will >> adjust as necessary. > > So I just looked at this with an eye to validating an XFS > implementation, and I came up with this list of stuff that the test > does not cover that I'd need to test in some way: > > - files with clean unwritten extents. Are they a hole or > data? What's SEEK_DATA supposed to return on layout like > hole-unwritten-data? i.e. needs to add fallocate to the > picture... > > - files with dirty unwritten extents (i.e. dirty in memory, > not on disk). They are most definitely data, and most > filesystems will need a separate lookup path to detect > dirty unwritten ranges because the state is kept > separately (page cache vs extent cache). Plenty of scope > for filesystem specific bugs here so needs a roubust test. > > - cold cache behaviour - all dirty data ranges the test > creates are hot in cache and not even forced to disk, so > it is not testing the no-page-cache-over-the-data-range > case. i.e. it tests delalloc state tracking but not > data-extent-already exists lookups during a seek. > > - assumes that allocation size is the block size and that > holes follows block size alignment. We already know that > ext4 does not follow that rule when doing small sparse > writes close together in a file, and XFS is also known to > fill holes when doing sparse writes past EOF. > > - only tests single block data extents =D1=95o doesn't cover > corner cases like skipping over multiple fragmented data > extents to the next hole. > Yeah I intentionally left out preallocated stuff because these are goin= g=20 to be implementation specific, so I was going to leave that for a later= =20 exercise when people actually start doing proper implementations. > Some more comments in line.... > >> +_cleanup() >> +{ >> + rm -f $tmp.* >> +} >> + >> +trap "_cleanup ; exit \$status" 0 1 2 3 15 >> + >> +# get standard environment, filters and checks >> +. ./common.rc >> +. ./common.filter >> + >> +# real QA test starts here >> +_supported_fs generic >> +_supported_os Linux >> + >> +testfile=3D$TEST_DIR/seek_test.$$ >> +logfile=3D$TEST_DIR/seek_test.$$.log > > The log file is usually named $seq.full, and doesn't get placed in > the filesystem being tested. It gets saved in the xfstests directory > along side $seq.out.bad for analysis whenteh test fails... > I only want it to see if SEEK_HOLE fails so I can say it didn't run. I= =20 followed the same example as the fiemap test that Eric wrote. >> +[ -x $here/src/seek-tester ] || _notrun "seek-tester not built" >> + >> +_cleanup() >> +{ >> + rm -f $testfile >> + rm -f $logfile >> +} >> +trap "_cleanup; exit \$status" 0 1 2 3 15 >> + >> +echo "Silence is golden" >> +$here/src/seek-tester -q $testfile 2>&1 | tee -a $logfile > > Personally I'd prefer the test to be a bit noisy about what it is > running, especially when there are so many subtests the single > invocation is running. It makes no difference to the run time ofthe > test, or the output when something fails, but it at least allows you > to run the test manually and see what it is doing easily... > Right, the problem with this test is it will run differently depending=20 on the implementation. I agree, I really like the noisy output tests,=20 but unfortunately if I run this test on ext4 where it currently treats=20 the entire file as data, and then run it on btrfs where it is smarter=20 and actually recognizes holes, we end up with two different outputs tha= t=20 are both correct. >> + >> +if grep -q "SEEK_HOLE is not supported" $logfile; then >> + _notrun "SEEK_HOLE/SEEK_DATA not supported by this kernel" >> +fi >> + >> +rm -f $logfile >> +rm -f $testfile >> + >> +status=3D0 ; exit >> diff --git a/255.out b/255.out >> new file mode 100644 >> index 0000000..7eefb82 >> --- /dev/null >> +++ b/255.out >> @@ -0,0 +1,2 @@ >> +QA output created by 255 >> +Silence is golden >> diff --git a/group b/group >> index 1f86075..c045e70 100644 >> --- a/group >> +++ b/group >> @@ -368,3 +368,4 @@ deprecated >> 252 auto quick prealloc >> 253 auto quick >> 254 auto quick >> +255 auto quick > > I'd suggest that rw and prealloc (once unwritten extent > testing is added) groups should also be defined for this test. > > Otherwise, the test code looks ok if a bit over-engineered.... > >> +struct testrec { >> + int test_num; >> + int (*test_func)(int fd, int testnum); >> + char *test_desc; >> +}; >> + >> +struct testrec seek_tests[] =3D { >> + { 1, test01, "Test basic support" }, >> + { 2, test02, "Test an empty file" }, >> + { 3, test03, "Test a full file" }, >> + { 4, test04, "Test file hole at beg, data at end" }, >> + { 5, test05, "Test file data at beg, hole at end" }, >> + { 6, test06, "Test file hole data hole data" }, > > So, to take from the hole punch test matrix, it covers a bunch more > file state transitions and cases that are just as relevant to > SEEK_HOLE/SEEK_DATA. Those cases are: > > # 1. into a hole > # 2. into allocated space > # 3. into unwritten space > # 4. hole -> data > # 5. hole -> unwritten > # 6. data -> hole > # 7. data -> unwritten > # 8. unwritten -> hole > # 9. unwritten -> data > # 10. hole -> data -> hole > # 11. data -> hole -> data > # 12. unwritten -> data -> unwritten > # 13. data -> unwritten -> data > # 14. data -> hole @ EOF > # 15. data -> hole @ 0 > # 16. data -> cache cold ->hole > # 17. data -> hole in single block file > > I thikn we also need to cover most of these same cases, right? And > SEEK_HOLE/SEEK data also need to explicitly separate the unwritten > tests into "clean unwritten" and "dirty unwritten" and cover the > transitions between regions of those states as well, right? > Yeah you are right, but again doing preallocated stuff is tricky, but I= =20 can expand it now if that's what we want. Thanks, Josef ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 13:19 ` Josef Bacik 0 siblings, 0 replies; 74+ messages in thread From: Josef Bacik @ 2011-06-29 13:19 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-fsdevel, viro, linux-kernel, linux-btrfs, xfs On 06/29/2011 02:53 AM, Dave Chinner wrote: > On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >> This is a test to make sure seek_data/seek_hole is acting like it does on >> Solaris. It will check to see if the fs supports finding a hole or not and will >> adjust as necessary. > > So I just looked at this with an eye to validating an XFS > implementation, and I came up with this list of stuff that the test > does not cover that I'd need to test in some way: > > - files with clean unwritten extents. Are they a hole or > data? What's SEEK_DATA supposed to return on layout like > hole-unwritten-data? i.e. needs to add fallocate to the > picture... > > - files with dirty unwritten extents (i.e. dirty in memory, > not on disk). They are most definitely data, and most > filesystems will need a separate lookup path to detect > dirty unwritten ranges because the state is kept > separately (page cache vs extent cache). Plenty of scope > for filesystem specific bugs here so needs a roubust test. > > - cold cache behaviour - all dirty data ranges the test > creates are hot in cache and not even forced to disk, so > it is not testing the no-page-cache-over-the-data-range > case. i.e. it tests delalloc state tracking but not > data-extent-already exists lookups during a seek. > > - assumes that allocation size is the block size and that > holes follows block size alignment. We already know that > ext4 does not follow that rule when doing small sparse > writes close together in a file, and XFS is also known to > fill holes when doing sparse writes past EOF. > > - only tests single block data extents ѕo doesn't cover > corner cases like skipping over multiple fragmented data > extents to the next hole. > Yeah I intentionally left out preallocated stuff because these are going to be implementation specific, so I was going to leave that for a later exercise when people actually start doing proper implementations. > Some more comments in line.... > >> +_cleanup() >> +{ >> + rm -f $tmp.* >> +} >> + >> +trap "_cleanup ; exit \$status" 0 1 2 3 15 >> + >> +# get standard environment, filters and checks >> +. ./common.rc >> +. ./common.filter >> + >> +# real QA test starts here >> +_supported_fs generic >> +_supported_os Linux >> + >> +testfile=$TEST_DIR/seek_test.$$ >> +logfile=$TEST_DIR/seek_test.$$.log > > The log file is usually named $seq.full, and doesn't get placed in > the filesystem being tested. It gets saved in the xfstests directory > along side $seq.out.bad for analysis whenteh test fails... > I only want it to see if SEEK_HOLE fails so I can say it didn't run. I followed the same example as the fiemap test that Eric wrote. >> +[ -x $here/src/seek-tester ] || _notrun "seek-tester not built" >> + >> +_cleanup() >> +{ >> + rm -f $testfile >> + rm -f $logfile >> +} >> +trap "_cleanup; exit \$status" 0 1 2 3 15 >> + >> +echo "Silence is golden" >> +$here/src/seek-tester -q $testfile 2>&1 | tee -a $logfile > > Personally I'd prefer the test to be a bit noisy about what it is > running, especially when there are so many subtests the single > invocation is running. It makes no difference to the run time ofthe > test, or the output when something fails, but it at least allows you > to run the test manually and see what it is doing easily... > Right, the problem with this test is it will run differently depending on the implementation. I agree, I really like the noisy output tests, but unfortunately if I run this test on ext4 where it currently treats the entire file as data, and then run it on btrfs where it is smarter and actually recognizes holes, we end up with two different outputs that are both correct. >> + >> +if grep -q "SEEK_HOLE is not supported" $logfile; then >> + _notrun "SEEK_HOLE/SEEK_DATA not supported by this kernel" >> +fi >> + >> +rm -f $logfile >> +rm -f $testfile >> + >> +status=0 ; exit >> diff --git a/255.out b/255.out >> new file mode 100644 >> index 0000000..7eefb82 >> --- /dev/null >> +++ b/255.out >> @@ -0,0 +1,2 @@ >> +QA output created by 255 >> +Silence is golden >> diff --git a/group b/group >> index 1f86075..c045e70 100644 >> --- a/group >> +++ b/group >> @@ -368,3 +368,4 @@ deprecated >> 252 auto quick prealloc >> 253 auto quick >> 254 auto quick >> +255 auto quick > > I'd suggest that rw and prealloc (once unwritten extent > testing is added) groups should also be defined for this test. > > Otherwise, the test code looks ok if a bit over-engineered.... > >> +struct testrec { >> + int test_num; >> + int (*test_func)(int fd, int testnum); >> + char *test_desc; >> +}; >> + >> +struct testrec seek_tests[] = { >> + { 1, test01, "Test basic support" }, >> + { 2, test02, "Test an empty file" }, >> + { 3, test03, "Test a full file" }, >> + { 4, test04, "Test file hole at beg, data at end" }, >> + { 5, test05, "Test file data at beg, hole at end" }, >> + { 6, test06, "Test file hole data hole data" }, > > So, to take from the hole punch test matrix, it covers a bunch more > file state transitions and cases that are just as relevant to > SEEK_HOLE/SEEK_DATA. Those cases are: > > # 1. into a hole > # 2. into allocated space > # 3. into unwritten space > # 4. hole -> data > # 5. hole -> unwritten > # 6. data -> hole > # 7. data -> unwritten > # 8. unwritten -> hole > # 9. unwritten -> data > # 10. hole -> data -> hole > # 11. data -> hole -> data > # 12. unwritten -> data -> unwritten > # 13. data -> unwritten -> data > # 14. data -> hole @ EOF > # 15. data -> hole @ 0 > # 16. data -> cache cold ->hole > # 17. data -> hole in single block file > > I thikn we also need to cover most of these same cases, right? And > SEEK_HOLE/SEEK data also need to explicitly separate the unwritten > tests into "clean unwritten" and "dirty unwritten" and cover the > transitions between regions of those states as well, right? > Yeah you are right, but again doing preallocated stuff is tricky, but I can expand it now if that's what we want. Thanks, Josef _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-06-29 13:19 ` Josef Bacik 0 siblings, 0 replies; 74+ messages in thread From: Josef Bacik @ 2011-06-29 13:19 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro On 06/29/2011 02:53 AM, Dave Chinner wrote: > On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >> This is a test to make sure seek_data/seek_hole is acting like it does on >> Solaris. It will check to see if the fs supports finding a hole or not and will >> adjust as necessary. > > So I just looked at this with an eye to validating an XFS > implementation, and I came up with this list of stuff that the test > does not cover that I'd need to test in some way: > > - files with clean unwritten extents. Are they a hole or > data? What's SEEK_DATA supposed to return on layout like > hole-unwritten-data? i.e. needs to add fallocate to the > picture... > > - files with dirty unwritten extents (i.e. dirty in memory, > not on disk). They are most definitely data, and most > filesystems will need a separate lookup path to detect > dirty unwritten ranges because the state is kept > separately (page cache vs extent cache). Plenty of scope > for filesystem specific bugs here so needs a roubust test. > > - cold cache behaviour - all dirty data ranges the test > creates are hot in cache and not even forced to disk, so > it is not testing the no-page-cache-over-the-data-range > case. i.e. it tests delalloc state tracking but not > data-extent-already exists lookups during a seek. > > - assumes that allocation size is the block size and that > holes follows block size alignment. We already know that > ext4 does not follow that rule when doing small sparse > writes close together in a file, and XFS is also known to > fill holes when doing sparse writes past EOF. > > - only tests single block data extents ѕo doesn't cover > corner cases like skipping over multiple fragmented data > extents to the next hole. > Yeah I intentionally left out preallocated stuff because these are going to be implementation specific, so I was going to leave that for a later exercise when people actually start doing proper implementations. > Some more comments in line.... > >> +_cleanup() >> +{ >> + rm -f $tmp.* >> +} >> + >> +trap "_cleanup ; exit \$status" 0 1 2 3 15 >> + >> +# get standard environment, filters and checks >> +. ./common.rc >> +. ./common.filter >> + >> +# real QA test starts here >> +_supported_fs generic >> +_supported_os Linux >> + >> +testfile=$TEST_DIR/seek_test.$$ >> +logfile=$TEST_DIR/seek_test.$$.log > > The log file is usually named $seq.full, and doesn't get placed in > the filesystem being tested. It gets saved in the xfstests directory > along side $seq.out.bad for analysis whenteh test fails... > I only want it to see if SEEK_HOLE fails so I can say it didn't run. I followed the same example as the fiemap test that Eric wrote. >> +[ -x $here/src/seek-tester ] || _notrun "seek-tester not built" >> + >> +_cleanup() >> +{ >> + rm -f $testfile >> + rm -f $logfile >> +} >> +trap "_cleanup; exit \$status" 0 1 2 3 15 >> + >> +echo "Silence is golden" >> +$here/src/seek-tester -q $testfile 2>&1 | tee -a $logfile > > Personally I'd prefer the test to be a bit noisy about what it is > running, especially when there are so many subtests the single > invocation is running. It makes no difference to the run time ofthe > test, or the output when something fails, but it at least allows you > to run the test manually and see what it is doing easily... > Right, the problem with this test is it will run differently depending on the implementation. I agree, I really like the noisy output tests, but unfortunately if I run this test on ext4 where it currently treats the entire file as data, and then run it on btrfs where it is smarter and actually recognizes holes, we end up with two different outputs that are both correct. >> + >> +if grep -q "SEEK_HOLE is not supported" $logfile; then >> + _notrun "SEEK_HOLE/SEEK_DATA not supported by this kernel" >> +fi >> + >> +rm -f $logfile >> +rm -f $testfile >> + >> +status=0 ; exit >> diff --git a/255.out b/255.out >> new file mode 100644 >> index 0000000..7eefb82 >> --- /dev/null >> +++ b/255.out >> @@ -0,0 +1,2 @@ >> +QA output created by 255 >> +Silence is golden >> diff --git a/group b/group >> index 1f86075..c045e70 100644 >> --- a/group >> +++ b/group >> @@ -368,3 +368,4 @@ deprecated >> 252 auto quick prealloc >> 253 auto quick >> 254 auto quick >> +255 auto quick > > I'd suggest that rw and prealloc (once unwritten extent > testing is added) groups should also be defined for this test. > > Otherwise, the test code looks ok if a bit over-engineered.... > >> +struct testrec { >> + int test_num; >> + int (*test_func)(int fd, int testnum); >> + char *test_desc; >> +}; >> + >> +struct testrec seek_tests[] = { >> + { 1, test01, "Test basic support" }, >> + { 2, test02, "Test an empty file" }, >> + { 3, test03, "Test a full file" }, >> + { 4, test04, "Test file hole at beg, data at end" }, >> + { 5, test05, "Test file data at beg, hole at end" }, >> + { 6, test06, "Test file hole data hole data" }, > > So, to take from the hole punch test matrix, it covers a bunch more > file state transitions and cases that are just as relevant to > SEEK_HOLE/SEEK_DATA. Those cases are: > > # 1. into a hole > # 2. into allocated space > # 3. into unwritten space > # 4. hole -> data > # 5. hole -> unwritten > # 6. data -> hole > # 7. data -> unwritten > # 8. unwritten -> hole > # 9. unwritten -> data > # 10. hole -> data -> hole > # 11. data -> hole -> data > # 12. unwritten -> data -> unwritten > # 13. data -> unwritten -> data > # 14. data -> hole @ EOF > # 15. data -> hole @ 0 > # 16. data -> cache cold ->hole > # 17. data -> hole in single block file > > I thikn we also need to cover most of these same cases, right? And > SEEK_HOLE/SEEK data also need to explicitly separate the unwritten > tests into "clean unwritten" and "dirty unwritten" and cover the > transitions between regions of those states as well, right? > Yeah you are right, but again doing preallocated stuff is tricky, but I can expand it now if that's what we want. Thanks, Josef ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-06-28 15:33 ` Josef Bacik @ 2011-08-25 6:06 ` Christoph Hellwig -1 siblings, 0 replies; 74+ messages in thread From: Christoph Hellwig @ 2011-08-25 6:06 UTC (permalink / raw) To: Josef Bacik; +Cc: linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: > This is a test to make sure seek_data/seek_hole is acting like it does on > Solaris. It will check to see if the fs supports finding a hole or not and will > adjust as necessary. Can you resend this with any updates that happened in the meantime? Dave also still had some comments about semantics, so it might be worth to incorporate that as well. ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-25 6:06 ` Christoph Hellwig 0 siblings, 0 replies; 74+ messages in thread From: Christoph Hellwig @ 2011-08-25 6:06 UTC (permalink / raw) To: Josef Bacik; +Cc: linux-kernel, xfs, viro, dchinner, linux-fsdevel, linux-btrfs On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: > This is a test to make sure seek_data/seek_hole is acting like it does on > Solaris. It will check to see if the fs supports finding a hole or not and will > adjust as necessary. Can you resend this with any updates that happened in the meantime? Dave also still had some comments about semantics, so it might be worth to incorporate that as well. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-08-25 6:06 ` Christoph Hellwig @ 2011-08-25 6:40 ` Dave Chinner -1 siblings, 0 replies; 74+ messages in thread From: Dave Chinner @ 2011-08-25 6:40 UTC (permalink / raw) To: Christoph Hellwig Cc: Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner On Thu, Aug 25, 2011 at 02:06:32AM -0400, Christoph Hellwig wrote: > On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: > > This is a test to make sure seek_data/seek_hole is acting like it does on > > Solaris. It will check to see if the fs supports finding a hole or not and will > > adjust as necessary. > > Can you resend this with any updates that happened in the meantime? > > Dave also still had some comments about semantics, so it might be worth > to incorporate that as well. The main questions I had when looking at this was how we should handle unwritten extents - the only answer I got was along the lines of "we'll deal with that once filesystems have implemented something". That's a bit of a chicken-and-egg situation, and doesn't help me decide what is the best thing to do. I don't want to have to re-implement this code when it's decided i did the wrong thing initially. The most basic question I really want answered is this: - is preallocated space a HOLE, or is it DATA? Whatever the answer, I think it should be consistently presented by all filesystems that support preallocation, and it should be encoded into the generic SEEK_HOLE/SEEK_DATA tests.... Answering that question then helps answer the more complex questions I had, like: - what does SEEK_DATA return when you have a file layout like "hole-prealloc-data"? Answers to that sort of question need to be known so we can write corner-case tests to correctly verify the filesystem implementation. I like to do better than present userspace with an interface that behaves vastly different depending on the underlying filesystem, but if the answer is "definition and implementation is entirely filesystem specific" then I'll just go make something up.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-25 6:40 ` Dave Chinner 0 siblings, 0 replies; 74+ messages in thread From: Dave Chinner @ 2011-08-25 6:40 UTC (permalink / raw) To: Christoph Hellwig Cc: linux-kernel, Josef Bacik, xfs, viro, dchinner, linux-fsdevel, linux-btrfs On Thu, Aug 25, 2011 at 02:06:32AM -0400, Christoph Hellwig wrote: > On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: > > This is a test to make sure seek_data/seek_hole is acting like it does on > > Solaris. It will check to see if the fs supports finding a hole or not and will > > adjust as necessary. > > Can you resend this with any updates that happened in the meantime? > > Dave also still had some comments about semantics, so it might be worth > to incorporate that as well. The main questions I had when looking at this was how we should handle unwritten extents - the only answer I got was along the lines of "we'll deal with that once filesystems have implemented something". That's a bit of a chicken-and-egg situation, and doesn't help me decide what is the best thing to do. I don't want to have to re-implement this code when it's decided i did the wrong thing initially. The most basic question I really want answered is this: - is preallocated space a HOLE, or is it DATA? Whatever the answer, I think it should be consistently presented by all filesystems that support preallocation, and it should be encoded into the generic SEEK_HOLE/SEEK_DATA tests.... Answering that question then helps answer the more complex questions I had, like: - what does SEEK_DATA return when you have a file layout like "hole-prealloc-data"? Answers to that sort of question need to be known so we can write corner-case tests to correctly verify the filesystem implementation. I like to do better than present userspace with an interface that behaves vastly different depending on the underlying filesystem, but if the answer is "definition and implementation is entirely filesystem specific" then I'll just go make something up.... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-08-25 6:40 ` Dave Chinner @ 2011-08-25 6:51 ` Andreas Dilger -1 siblings, 0 replies; 74+ messages in thread From: Andreas Dilger @ 2011-08-25 6:51 UTC (permalink / raw) To: Dave Chinner Cc: Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner On 2011-08-25, at 12:40 AM, Dave Chinner wrote: > On Thu, Aug 25, 2011 at 02:06:32AM -0400, Christoph Hellwig wrote: >> On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >>> This is a test to make sure seek_data/seek_hole is acting like it does on >>> Solaris. It will check to see if the fs supports finding a hole or not and will adjust as necessary. >> >> Can you resend this with any updates that happened in the meantime? >> >> Dave also still had some comments about semantics, so it might be worth >> to incorporate that as well. > > The main questions I had when looking at this was how we should > handle unwritten extents - the only answer I got was along the lines > of "we'll deal with that once filesystems have implemented > something". That's a bit of a chicken-and-egg situation, and doesn't > help me decide what is the best thing to do. I don't want to have to > re-implement this code when it's decided i did the wrong thing > initially. Let's first clarify what you mean by an unwritten extent? Do you mean a preallocated extent that returns 0 when read, or do you mean a delayed allocation extent that was written by the application that is still in memory but not yet written to disk? Unfortunately, ZFS has no concept of preallocated extents, so we can't look to it for precedent, but it definitely has delayed allocation. Possibly if UFS on Solaris has SEEK_HOLE and also preallocated extents (I have no idea) it could be tested? > The most basic question I really want answered is this: > > - is preallocated space a HOLE, or is it DATA? > > Whatever the answer, I think it should be consistently > presented by all filesystems that support preallocation, and it > should be encoded into the generic SEEK_HOLE/SEEK_DATA tests.... My thought would be that a preallocated extent is still a HOLE, because it doesn't contain data that an application actually cares about. On the other hand, a delalloc extent is DATA because it has something that an application cares about. The original reason SEEK_HOLE/SEEK_DATA were brought up over FIEMAP was "cp" being able to consistently access delalloc data that was only in the page cache, so if we don't get that right it will have been a pointless exercise. > Answering that question then helps answer the more complex questions > I had, like: > > - what does SEEK_DATA return when you have a file layout > like "hole-prealloc-data"? I would think only the "data" part, since that is what the definition of "SEEK_DATA" is IMHO. > Answers to that sort of question need to be known so we can write > corner-case tests to correctly verify the filesystem implementation. > > I like to do better than present userspace with an interface that > behaves vastly different depending on the underlying filesystem, but > if the answer is "definition and implementation is entirely > filesystem specific" then I'll just go make something up.... > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Cheers, Andreas ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-25 6:51 ` Andreas Dilger 0 siblings, 0 replies; 74+ messages in thread From: Andreas Dilger @ 2011-08-25 6:51 UTC (permalink / raw) To: Dave Chinner Cc: linux-kernel, xfs, Christoph Hellwig, Josef Bacik, dchinner, linux-fsdevel, linux-btrfs, viro On 2011-08-25, at 12:40 AM, Dave Chinner wrote: > On Thu, Aug 25, 2011 at 02:06:32AM -0400, Christoph Hellwig wrote: >> On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >>> This is a test to make sure seek_data/seek_hole is acting like it does on >>> Solaris. It will check to see if the fs supports finding a hole or not and will adjust as necessary. >> >> Can you resend this with any updates that happened in the meantime? >> >> Dave also still had some comments about semantics, so it might be worth >> to incorporate that as well. > > The main questions I had when looking at this was how we should > handle unwritten extents - the only answer I got was along the lines > of "we'll deal with that once filesystems have implemented > something". That's a bit of a chicken-and-egg situation, and doesn't > help me decide what is the best thing to do. I don't want to have to > re-implement this code when it's decided i did the wrong thing > initially. Let's first clarify what you mean by an unwritten extent? Do you mean a preallocated extent that returns 0 when read, or do you mean a delayed allocation extent that was written by the application that is still in memory but not yet written to disk? Unfortunately, ZFS has no concept of preallocated extents, so we can't look to it for precedent, but it definitely has delayed allocation. Possibly if UFS on Solaris has SEEK_HOLE and also preallocated extents (I have no idea) it could be tested? > The most basic question I really want answered is this: > > - is preallocated space a HOLE, or is it DATA? > > Whatever the answer, I think it should be consistently > presented by all filesystems that support preallocation, and it > should be encoded into the generic SEEK_HOLE/SEEK_DATA tests.... My thought would be that a preallocated extent is still a HOLE, because it doesn't contain data that an application actually cares about. On the other hand, a delalloc extent is DATA because it has something that an application cares about. The original reason SEEK_HOLE/SEEK_DATA were brought up over FIEMAP was "cp" being able to consistently access delalloc data that was only in the page cache, so if we don't get that right it will have been a pointless exercise. > Answering that question then helps answer the more complex questions > I had, like: > > - what does SEEK_DATA return when you have a file layout > like "hole-prealloc-data"? I would think only the "data" part, since that is what the definition of "SEEK_DATA" is IMHO. > Answers to that sort of question need to be known so we can write > corner-case tests to correctly verify the filesystem implementation. > > I like to do better than present userspace with an interface that > behaves vastly different depending on the underlying filesystem, but > if the answer is "definition and implementation is entirely > filesystem specific" then I'll just go make something up.... > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Cheers, Andreas _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-08-25 6:51 ` Andreas Dilger @ 2011-08-26 1:35 ` Dave Chinner -1 siblings, 0 replies; 74+ messages in thread From: Dave Chinner @ 2011-08-26 1:35 UTC (permalink / raw) To: Andreas Dilger Cc: Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner On Thu, Aug 25, 2011 at 12:51:56AM -0600, Andreas Dilger wrote: > On 2011-08-25, at 12:40 AM, Dave Chinner wrote: > > On Thu, Aug 25, 2011 at 02:06:32AM -0400, Christoph Hellwig wrote: > >> On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: > >>> This is a test to make sure seek_data/seek_hole is acting like it does on > >>> Solaris. It will check to see if the fs supports finding a hole or not and will adjust as necessary. > >> > >> Can you resend this with any updates that happened in the meantime? > >> > >> Dave also still had some comments about semantics, so it might be worth > >> to incorporate that as well. > > > > The main questions I had when looking at this was how we should > > handle unwritten extents - the only answer I got was along the lines > > of "we'll deal with that once filesystems have implemented > > something". That's a bit of a chicken-and-egg situation, and doesn't > > help me decide what is the best thing to do. I don't want to have to > > re-implement this code when it's decided i did the wrong thing > > initially. > > Let's first clarify what you mean by an unwritten extent? Do you mean a > preallocated extent that returns 0 when read, Exactly that. > or do you mean a delayed > allocation extent that was written by the application that is still in > memory but not yet written to disk? That's not an unwritten extent - that's a delayed allocation extent ;) > Unfortunately, ZFS has no concept of preallocated extents, so we can't > look to it for precedent, but it definitely has delayed allocation. > Possibly if UFS on Solaris has SEEK_HOLE and also preallocated extents > (I have no idea) it could be tested? > > > The most basic question I really want answered is this: > > > > - is preallocated space a HOLE, or is it DATA? > > > > Whatever the answer, I think it should be consistently > > presented by all filesystems that support preallocation, and it > > should be encoded into the generic SEEK_HOLE/SEEK_DATA tests.... > > My thought would be that a preallocated extent is still a HOLE, because > it doesn't contain data that an application actually cares about. On > the other hand, a delalloc extent is DATA because it has something that > an application cares about. OK, that's the way I'd expect to treat both preallocated and delalloc space. > > Answering that question then helps answer the more complex questions > > I had, like: > > > > - what does SEEK_DATA return when you have a file layout > > like "hole-prealloc-data"? > > I would think only the "data" part, since that is what the definition > of "SEEK_DATA" is IMHO. Agreed, that's the way I'd interpret it, too. So perhaps we need to ensure that this interpretation is actually tested by this test? How about some definitions to work by: Data: a range of the file that contains valid data, regardless of whether it exists in memory or on disk. The valid data can be preceeded and/or followed by an arbitrary number of zero bytes dependent on the underlying implementation of hole detection. Hole: a range of the file that contains no data or is made up entirely of NULL (zero) data. Holes include preallocated ranges of files that have not had actual data written to them. Does that make sense? It has sufficient flexibility in it for the existing generic "non-implementation", allows for filesystems to define their own hole detection boundaries (e.g. filesystem block size), and effectively defines how preallocated ranges from fallocate() should be treated (i.e. as holes). If we can agree on those definitions, I think that we should document them in both the kernel and the man page that defines SEEK_HOLE/SEEK_DATA so everyone is on the same page... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-26 1:35 ` Dave Chinner 0 siblings, 0 replies; 74+ messages in thread From: Dave Chinner @ 2011-08-26 1:35 UTC (permalink / raw) To: Andreas Dilger Cc: linux-kernel, xfs, Christoph Hellwig, Josef Bacik, dchinner, linux-fsdevel, linux-btrfs, viro On Thu, Aug 25, 2011 at 12:51:56AM -0600, Andreas Dilger wrote: > On 2011-08-25, at 12:40 AM, Dave Chinner wrote: > > On Thu, Aug 25, 2011 at 02:06:32AM -0400, Christoph Hellwig wrote: > >> On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: > >>> This is a test to make sure seek_data/seek_hole is acting like it does on > >>> Solaris. It will check to see if the fs supports finding a hole or not and will adjust as necessary. > >> > >> Can you resend this with any updates that happened in the meantime? > >> > >> Dave also still had some comments about semantics, so it might be worth > >> to incorporate that as well. > > > > The main questions I had when looking at this was how we should > > handle unwritten extents - the only answer I got was along the lines > > of "we'll deal with that once filesystems have implemented > > something". That's a bit of a chicken-and-egg situation, and doesn't > > help me decide what is the best thing to do. I don't want to have to > > re-implement this code when it's decided i did the wrong thing > > initially. > > Let's first clarify what you mean by an unwritten extent? Do you mean a > preallocated extent that returns 0 when read, Exactly that. > or do you mean a delayed > allocation extent that was written by the application that is still in > memory but not yet written to disk? That's not an unwritten extent - that's a delayed allocation extent ;) > Unfortunately, ZFS has no concept of preallocated extents, so we can't > look to it for precedent, but it definitely has delayed allocation. > Possibly if UFS on Solaris has SEEK_HOLE and also preallocated extents > (I have no idea) it could be tested? > > > The most basic question I really want answered is this: > > > > - is preallocated space a HOLE, or is it DATA? > > > > Whatever the answer, I think it should be consistently > > presented by all filesystems that support preallocation, and it > > should be encoded into the generic SEEK_HOLE/SEEK_DATA tests.... > > My thought would be that a preallocated extent is still a HOLE, because > it doesn't contain data that an application actually cares about. On > the other hand, a delalloc extent is DATA because it has something that > an application cares about. OK, that's the way I'd expect to treat both preallocated and delalloc space. > > Answering that question then helps answer the more complex questions > > I had, like: > > > > - what does SEEK_DATA return when you have a file layout > > like "hole-prealloc-data"? > > I would think only the "data" part, since that is what the definition > of "SEEK_DATA" is IMHO. Agreed, that's the way I'd interpret it, too. So perhaps we need to ensure that this interpretation is actually tested by this test? How about some definitions to work by: Data: a range of the file that contains valid data, regardless of whether it exists in memory or on disk. The valid data can be preceeded and/or followed by an arbitrary number of zero bytes dependent on the underlying implementation of hole detection. Hole: a range of the file that contains no data or is made up entirely of NULL (zero) data. Holes include preallocated ranges of files that have not had actual data written to them. Does that make sense? It has sufficient flexibility in it for the existing generic "non-implementation", allows for filesystems to define their own hole detection boundaries (e.g. filesystem block size), and effectively defines how preallocated ranges from fallocate() should be treated (i.e. as holes). If we can agree on those definitions, I think that we should document them in both the kernel and the man page that defines SEEK_HOLE/SEEK_DATA so everyone is on the same page... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-08-26 1:35 ` Dave Chinner (?) @ 2011-08-26 6:24 ` Marco Stornelli -1 siblings, 0 replies; 74+ messages in thread From: Marco Stornelli @ 2011-08-26 6:24 UTC (permalink / raw) To: Dave Chinner Cc: Andreas Dilger, Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner 2011/8/26 Dave Chinner <david@fromorbit.com>: > On Thu, Aug 25, 2011 at 12:51:56AM -0600, Andreas Dilger wrote: >> On 2011-08-25, at 12:40 AM, Dave Chinner wrote: >> > On Thu, Aug 25, 2011 at 02:06:32AM -0400, Christoph Hellwig wrote: >> >> On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >> >>> This is a test to make sure seek_data/seek_hole is acting like i= t does on >> >>> Solaris. =A0It will check to see if the fs supports finding a ho= le or not and will adjust as necessary. >> >> >> >> Can you resend this with any updates that happened in the meantim= e? >> >> >> >> Dave also still had some comments about semantics, so it might be= worth >> >> to incorporate that as well. >> > >> > The main questions I had when looking at this was how we should >> > handle unwritten extents - the only answer I got was along the lin= es >> > of "we'll deal with that once filesystems have implemented >> > something". That's a bit of a chicken-and-egg situation, and doesn= 't >> > help me decide what is the best thing to do. I don't want to have = to >> > re-implement this code when it's decided i did the wrong thing >> > initially. >> >> Let's first clarify what you mean by an unwritten extent? =A0Do you = mean a >> preallocated extent that returns 0 when read, > > Exactly that. > >> or do you mean a delayed >> allocation extent that was written by the application that is still = in >> memory but not yet written to disk? > > That's not an unwritten extent - that's a delayed allocation extent ;= ) > >> Unfortunately, ZFS has no concept of preallocated extents, so we can= 't >> look to it for precedent, but it definitely has delayed allocation. >> Possibly if UFS on Solaris has SEEK_HOLE and also preallocated exten= ts >> (I have no idea) it could be tested? >> >> > The most basic question I really want answered is this: >> > >> > =A0 =A0 - is preallocated space a HOLE, or is it DATA? >> > >> > Whatever the answer, I think it should be consistently >> > presented by all filesystems that support preallocation, and it >> > should be encoded into the generic SEEK_HOLE/SEEK_DATA tests.... >> >> My thought would be that a preallocated extent is still a HOLE, beca= use >> it doesn't contain data that an application actually cares about. =A0= On >> the other hand, a delalloc extent is DATA because it has something t= hat >> an application cares about. > > OK, that's the way I'd expect to treat both preallocated and > delalloc space. > >> > Answering that question then helps answer the more complex questio= ns >> > I had, like: >> > >> > =A0 =A0 - what does SEEK_DATA return when you have a file layout >> > =A0 =A0 =A0 like "hole-prealloc-data"? >> >> I would think only the "data" part, since that is what the definitio= n >> of "SEEK_DATA" is IMHO. > > Agreed, that's the way I'd interpret it, too. So perhaps we need to > ensure that this interpretation is actually tested by this test? > > How about some definitions to work by: > > Data: a range of the file that contains valid data, regardless of > whether it exists in memory or on disk. The valid data can be > preceeded and/or followed by an arbitrary number of zero bytes > dependent on the underlying implementation of hole detection. > > Hole: a range of the file that contains no data or is made up > entirely of =A0NULL (zero) data. Holes include preallocated ranges of > files that have not had actual data written to them. > > Does that make sense? It has sufficient flexibility in it for the No for me. A hole is made up of zero data? It's a strange definition for me. A hole is simply no data. With this definition a hole can be start from a block with data but partially filled, and fs should check instead of presence of a block, the data content. I don't like it very much. For the fallocate case, we need only a simple rule: no check is done beyond eof. At this point it's clear that we can check the file block allocation till i_size, don't care if there are other blocks allocated. I don't know if it's applicable even for unwritten extents case. Marco ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-26 6:24 ` Marco Stornelli 0 siblings, 0 replies; 74+ messages in thread From: Marco Stornelli @ 2011-08-26 6:24 UTC (permalink / raw) To: Dave Chinner Cc: Andreas Dilger, linux-kernel, xfs, Christoph Hellwig, Josef Bacik, dchinner, linux-fsdevel, linux-btrfs, viro 2011/8/26 Dave Chinner <david@fromorbit.com>: > On Thu, Aug 25, 2011 at 12:51:56AM -0600, Andreas Dilger wrote: >> On 2011-08-25, at 12:40 AM, Dave Chinner wrote: >> > On Thu, Aug 25, 2011 at 02:06:32AM -0400, Christoph Hellwig wrote: >> >> On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >> >>> This is a test to make sure seek_data/seek_hole is acting like it does on >> >>> Solaris. It will check to see if the fs supports finding a hole or not and will adjust as necessary. >> >> >> >> Can you resend this with any updates that happened in the meantime? >> >> >> >> Dave also still had some comments about semantics, so it might be worth >> >> to incorporate that as well. >> > >> > The main questions I had when looking at this was how we should >> > handle unwritten extents - the only answer I got was along the lines >> > of "we'll deal with that once filesystems have implemented >> > something". That's a bit of a chicken-and-egg situation, and doesn't >> > help me decide what is the best thing to do. I don't want to have to >> > re-implement this code when it's decided i did the wrong thing >> > initially. >> >> Let's first clarify what you mean by an unwritten extent? Do you mean a >> preallocated extent that returns 0 when read, > > Exactly that. > >> or do you mean a delayed >> allocation extent that was written by the application that is still in >> memory but not yet written to disk? > > That's not an unwritten extent - that's a delayed allocation extent ;) > >> Unfortunately, ZFS has no concept of preallocated extents, so we can't >> look to it for precedent, but it definitely has delayed allocation. >> Possibly if UFS on Solaris has SEEK_HOLE and also preallocated extents >> (I have no idea) it could be tested? >> >> > The most basic question I really want answered is this: >> > >> > - is preallocated space a HOLE, or is it DATA? >> > >> > Whatever the answer, I think it should be consistently >> > presented by all filesystems that support preallocation, and it >> > should be encoded into the generic SEEK_HOLE/SEEK_DATA tests.... >> >> My thought would be that a preallocated extent is still a HOLE, because >> it doesn't contain data that an application actually cares about. On >> the other hand, a delalloc extent is DATA because it has something that >> an application cares about. > > OK, that's the way I'd expect to treat both preallocated and > delalloc space. > >> > Answering that question then helps answer the more complex questions >> > I had, like: >> > >> > - what does SEEK_DATA return when you have a file layout >> > like "hole-prealloc-data"? >> >> I would think only the "data" part, since that is what the definition >> of "SEEK_DATA" is IMHO. > > Agreed, that's the way I'd interpret it, too. So perhaps we need to > ensure that this interpretation is actually tested by this test? > > How about some definitions to work by: > > Data: a range of the file that contains valid data, regardless of > whether it exists in memory or on disk. The valid data can be > preceeded and/or followed by an arbitrary number of zero bytes > dependent on the underlying implementation of hole detection. > > Hole: a range of the file that contains no data or is made up > entirely of NULL (zero) data. Holes include preallocated ranges of > files that have not had actual data written to them. > > Does that make sense? It has sufficient flexibility in it for the No for me. A hole is made up of zero data? It's a strange definition for me. A hole is simply no data. With this definition a hole can be start from a block with data but partially filled, and fs should check instead of presence of a block, the data content. I don't like it very much. For the fallocate case, we need only a simple rule: no check is done beyond eof. At this point it's clear that we can check the file block allocation till i_size, don't care if there are other blocks allocated. I don't know if it's applicable even for unwritten extents case. Marco _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-26 6:24 ` Marco Stornelli 0 siblings, 0 replies; 74+ messages in thread From: Marco Stornelli @ 2011-08-26 6:24 UTC (permalink / raw) To: Dave Chinner Cc: Andreas Dilger, Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner 2011/8/26 Dave Chinner <david@fromorbit.com>: > On Thu, Aug 25, 2011 at 12:51:56AM -0600, Andreas Dilger wrote: >> On 2011-08-25, at 12:40 AM, Dave Chinner wrote: >> > On Thu, Aug 25, 2011 at 02:06:32AM -0400, Christoph Hellwig wrote: >> >> On Tue, Jun 28, 2011 at 11:33:19AM -0400, Josef Bacik wrote: >> >>> This is a test to make sure seek_data/seek_hole is acting like it does on >> >>> Solaris. It will check to see if the fs supports finding a hole or not and will adjust as necessary. >> >> >> >> Can you resend this with any updates that happened in the meantime? >> >> >> >> Dave also still had some comments about semantics, so it might be worth >> >> to incorporate that as well. >> > >> > The main questions I had when looking at this was how we should >> > handle unwritten extents - the only answer I got was along the lines >> > of "we'll deal with that once filesystems have implemented >> > something". That's a bit of a chicken-and-egg situation, and doesn't >> > help me decide what is the best thing to do. I don't want to have to >> > re-implement this code when it's decided i did the wrong thing >> > initially. >> >> Let's first clarify what you mean by an unwritten extent? Do you mean a >> preallocated extent that returns 0 when read, > > Exactly that. > >> or do you mean a delayed >> allocation extent that was written by the application that is still in >> memory but not yet written to disk? > > That's not an unwritten extent - that's a delayed allocation extent ;) > >> Unfortunately, ZFS has no concept of preallocated extents, so we can't >> look to it for precedent, but it definitely has delayed allocation. >> Possibly if UFS on Solaris has SEEK_HOLE and also preallocated extents >> (I have no idea) it could be tested? >> >> > The most basic question I really want answered is this: >> > >> > - is preallocated space a HOLE, or is it DATA? >> > >> > Whatever the answer, I think it should be consistently >> > presented by all filesystems that support preallocation, and it >> > should be encoded into the generic SEEK_HOLE/SEEK_DATA tests.... >> >> My thought would be that a preallocated extent is still a HOLE, because >> it doesn't contain data that an application actually cares about. On >> the other hand, a delalloc extent is DATA because it has something that >> an application cares about. > > OK, that's the way I'd expect to treat both preallocated and > delalloc space. > >> > Answering that question then helps answer the more complex questions >> > I had, like: >> > >> > - what does SEEK_DATA return when you have a file layout >> > like "hole-prealloc-data"? >> >> I would think only the "data" part, since that is what the definition >> of "SEEK_DATA" is IMHO. > > Agreed, that's the way I'd interpret it, too. So perhaps we need to > ensure that this interpretation is actually tested by this test? > > How about some definitions to work by: > > Data: a range of the file that contains valid data, regardless of > whether it exists in memory or on disk. The valid data can be > preceeded and/or followed by an arbitrary number of zero bytes > dependent on the underlying implementation of hole detection. > > Hole: a range of the file that contains no data or is made up > entirely of NULL (zero) data. Holes include preallocated ranges of > files that have not had actual data written to them. > > Does that make sense? It has sufficient flexibility in it for the No for me. A hole is made up of zero data? It's a strange definition for me. A hole is simply no data. With this definition a hole can be start from a block with data but partially filled, and fs should check instead of presence of a block, the data content. I don't like it very much. For the fallocate case, we need only a simple rule: no check is done beyond eof. At this point it's clear that we can check the file block allocation till i_size, don't care if there are other blocks allocated. I don't know if it's applicable even for unwritten extents case. Marco ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-08-26 6:24 ` Marco Stornelli (?) (?) @ 2011-08-26 14:41 ` Zach Brown -1 siblings, 0 replies; 74+ messages in thread From: Zach Brown @ 2011-08-26 14:41 UTC (permalink / raw) To: Marco Stornelli Cc: Dave Chinner, Andreas Dilger, Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner > > Hole: a range of the file that contains no data or is made up > > entirely of =A0NULL (zero) data. Holes include preallocated ranges = of > > files that have not had actual data written to them. > No for me. A hole is made up of zero data? It's a strange definition > for me. It's a very natural definition for me. It mirrors the behaviour of read() of sparse data inside i_size that file system authors already have to consider. It's also a reminder for people that this interface is about avoiding reading zeros. Systems that track contents can do this for files that had tons of zeros written. The data is there but the app is specifically asking us to skip it by using SEEK_DATA. - z -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" = 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] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-26 14:41 ` Zach Brown 0 siblings, 0 replies; 74+ messages in thread From: Zach Brown @ 2011-08-26 14:41 UTC (permalink / raw) To: Marco Stornelli Cc: Andreas Dilger, linux-kernel, xfs, Christoph Hellwig, linux-btrfs, dchinner, linux-fsdevel, Josef Bacik, viro > > Hole: a range of the file that contains no data or is made up > > entirely of NULL (zero) data. Holes include preallocated ranges of > > files that have not had actual data written to them. > No for me. A hole is made up of zero data? It's a strange definition > for me. It's a very natural definition for me. It mirrors the behaviour of read() of sparse data inside i_size that file system authors already have to consider. It's also a reminder for people that this interface is about avoiding reading zeros. Systems that track contents can do this for files that had tons of zeros written. The data is there but the app is specifically asking us to skip it by using SEEK_DATA. - z _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-26 14:41 ` Zach Brown 0 siblings, 0 replies; 74+ messages in thread From: Zach Brown @ 2011-08-26 14:41 UTC (permalink / raw) To: Marco Stornelli Cc: Dave Chinner, Andreas Dilger, Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner > > Hole: a range of the file that contains no data or is made up > > entirely of NULL (zero) data. Holes include preallocated ranges of > > files that have not had actual data written to them. > No for me. A hole is made up of zero data? It's a strange definition > for me. It's a very natural definition for me. It mirrors the behaviour of read() of sparse data inside i_size that file system authors already have to consider. It's also a reminder for people that this interface is about avoiding reading zeros. Systems that track contents can do this for files that had tons of zeros written. The data is there but the app is specifically asking us to skip it by using SEEK_DATA. - z -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 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] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-26 14:41 ` Zach Brown 0 siblings, 0 replies; 74+ messages in thread From: Zach Brown @ 2011-08-26 14:41 UTC (permalink / raw) To: Marco Stornelli Cc: Dave Chinner, Andreas Dilger, Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner > > Hole: a range of the file that contains no data or is made up > > entirely of NULL (zero) data. Holes include preallocated ranges of > > files that have not had actual data written to them. > No for me. A hole is made up of zero data? It's a strange definition > for me. It's a very natural definition for me. It mirrors the behaviour of read() of sparse data inside i_size that file system authors already have to consider. It's also a reminder for people that this interface is about avoiding reading zeros. Systems that track contents can do this for files that had tons of zeros written. The data is there but the app is specifically asking us to skip it by using SEEK_DATA. - z ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-08-26 14:41 ` Zach Brown @ 2011-08-27 8:30 ` Marco Stornelli -1 siblings, 0 replies; 74+ messages in thread From: Marco Stornelli @ 2011-08-27 8:30 UTC (permalink / raw) To: Zach Brown Cc: Dave Chinner, Andreas Dilger, Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner Il 26/08/2011 16:41, Zach Brown ha scritto: >>> Hole: a range of the file that contains no data or is made up >>> entirely of NULL (zero) data. Holes include preallocated ranges of >>> files that have not had actual data written to them. > >> No for me. A hole is made up of zero data? It's a strange definition >> for me. > > It's a very natural definition for me. It mirrors the behaviour of > read() of sparse data inside i_size that file system authors already > have to consider. > > It's also a reminder for people that this interface is about avoiding > reading zeros. Systems that track contents can do this for files that > had tons of zeros written. The data is there but the app is > specifically asking us to skip it by using SEEK_DATA. > > - z > I think we need to consider a hole and "data not present/not written yet" as two different cases even they are related. For example, if I do an fallocate without keep size option, then I do a read, I have the same behavior of sparse data inside i_size, but the blocks are allocated so no sparse data in this case. Simply there are no difference from app point of view. Marco ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-27 8:30 ` Marco Stornelli 0 siblings, 0 replies; 74+ messages in thread From: Marco Stornelli @ 2011-08-27 8:30 UTC (permalink / raw) To: Zach Brown Cc: Andreas Dilger, linux-kernel, xfs, Christoph Hellwig, linux-btrfs, dchinner, linux-fsdevel, Josef Bacik, viro Il 26/08/2011 16:41, Zach Brown ha scritto: >>> Hole: a range of the file that contains no data or is made up >>> entirely of NULL (zero) data. Holes include preallocated ranges of >>> files that have not had actual data written to them. > >> No for me. A hole is made up of zero data? It's a strange definition >> for me. > > It's a very natural definition for me. It mirrors the behaviour of > read() of sparse data inside i_size that file system authors already > have to consider. > > It's also a reminder for people that this interface is about avoiding > reading zeros. Systems that track contents can do this for files that > had tons of zeros written. The data is there but the app is > specifically asking us to skip it by using SEEK_DATA. > > - z > I think we need to consider a hole and "data not present/not written yet" as two different cases even they are related. For example, if I do an fallocate without keep size option, then I do a read, I have the same behavior of sparse data inside i_size, but the blocks are allocated so no sparse data in this case. Simply there are no difference from app point of view. Marco _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-08-27 8:30 ` Marco Stornelli @ 2011-08-28 10:17 ` Marco Stornelli -1 siblings, 0 replies; 74+ messages in thread From: Marco Stornelli @ 2011-08-28 10:17 UTC (permalink / raw) To: Zach Brown Cc: Dave Chinner, Andreas Dilger, Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner Il 27/08/2011 10:30, Marco Stornelli ha scritto: > Il 26/08/2011 16:41, Zach Brown ha scritto: >>>> Hole: a range of the file that contains no data or is made up >>>> entirely of NULL (zero) data. Holes include preallocated ranges of >>>> files that have not had actual data written to them. >> >>> No for me. A hole is made up of zero data? It's a strange definition >>> for me. >> >> It's a very natural definition for me. It mirrors the behaviour of >> read() of sparse data inside i_size that file system authors already >> have to consider. >> >> It's also a reminder for people that this interface is about avoiding >> reading zeros. Systems that track contents can do this for files that >> had tons of zeros written. The data is there but the app is >> specifically asking us to skip it by using SEEK_DATA. >> >> - z >> > > I think we need to consider a hole and "data not present/not written > yet" as two different cases even they are related. For example, if I do > an fallocate without keep size option, then I do a read, I have the same > behavior of sparse data inside i_size, but the blocks are allocated so > no sparse data in this case. Simply there are no difference from app > point of view. > > Marco Please don't care about the last part, when reading in this case the app will have a return value different from zero obviously, I was under the effect of a beer :) However I'd add to the definition, that we consider holes only inside i_size, as Zack said. In this way, we haven't got any ambiguity for preallocated space beyond eof. Marco ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-28 10:17 ` Marco Stornelli 0 siblings, 0 replies; 74+ messages in thread From: Marco Stornelli @ 2011-08-28 10:17 UTC (permalink / raw) To: Zach Brown Cc: Andreas Dilger, linux-kernel, xfs, Christoph Hellwig, linux-btrfs, dchinner, linux-fsdevel, Josef Bacik, viro Il 27/08/2011 10:30, Marco Stornelli ha scritto: > Il 26/08/2011 16:41, Zach Brown ha scritto: >>>> Hole: a range of the file that contains no data or is made up >>>> entirely of NULL (zero) data. Holes include preallocated ranges of >>>> files that have not had actual data written to them. >> >>> No for me. A hole is made up of zero data? It's a strange definition >>> for me. >> >> It's a very natural definition for me. It mirrors the behaviour of >> read() of sparse data inside i_size that file system authors already >> have to consider. >> >> It's also a reminder for people that this interface is about avoiding >> reading zeros. Systems that track contents can do this for files that >> had tons of zeros written. The data is there but the app is >> specifically asking us to skip it by using SEEK_DATA. >> >> - z >> > > I think we need to consider a hole and "data not present/not written > yet" as two different cases even they are related. For example, if I do > an fallocate without keep size option, then I do a read, I have the same > behavior of sparse data inside i_size, but the blocks are allocated so > no sparse data in this case. Simply there are no difference from app > point of view. > > Marco Please don't care about the last part, when reading in this case the app will have a return value different from zero obviously, I was under the effect of a beer :) However I'd add to the definition, that we consider holes only inside i_size, as Zack said. In this way, we haven't got any ambiguity for preallocated space beyond eof. Marco _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-08-27 8:30 ` Marco Stornelli @ 2011-08-30 17:42 ` Sunil Mushran -1 siblings, 0 replies; 74+ messages in thread From: Sunil Mushran @ 2011-08-30 17:42 UTC (permalink / raw) To: Marco Stornelli Cc: Zach Brown, Dave Chinner, Andreas Dilger, Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner On 08/27/2011 01:30 AM, Marco Stornelli wrote: > Il 26/08/2011 16:41, Zach Brown ha scritto: >>>> Hole: a range of the file that contains no data or is made up >>>> entirely of NULL (zero) data. Holes include preallocated ranges of >>>> files that have not had actual data written to them. >> >>> No for me. A hole is made up of zero data? It's a strange definition >>> for me. >> >> It's a very natural definition for me. It mirrors the behaviour of >> read() of sparse data inside i_size that file system authors already >> have to consider. >> >> It's also a reminder for people that this interface is about avoiding >> reading zeros. Systems that track contents can do this for files that >> had tons of zeros written. The data is there but the app is >> specifically asking us to skip it by using SEEK_DATA. >> >> - z >> > > I think we need to consider a hole and "data not present/not written yet" as two different cases even they are related. For example, if I do an fallocate without keep size option, then I do a read, I have the same behavior of sparse data inside i_size, but the blocks are allocated so no sparse data in this case. Simply there are no difference from app point of view. Exactly. That's why seek_hole should identify them alike, if possible. But that should not be a requirement because the sole aim here is to improve performance. Identifying a hole as data is not the end of the world. In some cases it may be more efficient. We just have to ensure that we don't identify data as a hole. BTW, we still have the fiemap interface that allows users to identify unwritten extents, etc. Use that if you want that kind of detail. ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-30 17:42 ` Sunil Mushran 0 siblings, 0 replies; 74+ messages in thread From: Sunil Mushran @ 2011-08-30 17:42 UTC (permalink / raw) To: Marco Stornelli Cc: Andreas Dilger, linux-kernel, xfs, Christoph Hellwig, linux-btrfs, dchinner, linux-fsdevel, Zach Brown, Josef Bacik, viro On 08/27/2011 01:30 AM, Marco Stornelli wrote: > Il 26/08/2011 16:41, Zach Brown ha scritto: >>>> Hole: a range of the file that contains no data or is made up >>>> entirely of NULL (zero) data. Holes include preallocated ranges of >>>> files that have not had actual data written to them. >> >>> No for me. A hole is made up of zero data? It's a strange definition >>> for me. >> >> It's a very natural definition for me. It mirrors the behaviour of >> read() of sparse data inside i_size that file system authors already >> have to consider. >> >> It's also a reminder for people that this interface is about avoiding >> reading zeros. Systems that track contents can do this for files that >> had tons of zeros written. The data is there but the app is >> specifically asking us to skip it by using SEEK_DATA. >> >> - z >> > > I think we need to consider a hole and "data not present/not written yet" as two different cases even they are related. For example, if I do an fallocate without keep size option, then I do a read, I have the same behavior of sparse data inside i_size, but the blocks are allocated so no sparse data in this case. Simply there are no difference from app point of view. Exactly. That's why seek_hole should identify them alike, if possible. But that should not be a requirement because the sole aim here is to improve performance. Identifying a hole as data is not the end of the world. In some cases it may be more efficient. We just have to ensure that we don't identify data as a hole. BTW, we still have the fiemap interface that allows users to identify unwritten extents, etc. Use that if you want that kind of detail. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-08-26 1:35 ` Dave Chinner @ 2011-08-31 1:17 ` Sunil Mushran -1 siblings, 0 replies; 74+ messages in thread From: Sunil Mushran @ 2011-08-31 1:17 UTC (permalink / raw) To: Dave Chinner Cc: Andreas Dilger, Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner On 08/25/2011 06:35 PM, Dave Chinner wrote: > Agreed, that's the way I'd interpret it, too. So perhaps we need to > ensure that this interpretation is actually tested by this test? > > How about some definitions to work by: > > Data: a range of the file that contains valid data, regardless of > whether it exists in memory or on disk. The valid data can be > preceeded and/or followed by an arbitrary number of zero bytes > dependent on the underlying implementation of hole detection. > > Hole: a range of the file that contains no data or is made up > entirely of NULL (zero) data. Holes include preallocated ranges of > files that have not had actual data written to them. > > Does that make sense? It has sufficient flexibility in it for the > existing generic "non-implementation", allows for filesystems to > define their own hole detection boundaries (e.g. filesystem block > size), and effectively defines how preallocated ranges from > fallocate() should be treated (i.e. as holes). If we can agree on > those definitions, I think that we should document them in both the > kernel and the man page that defines SEEK_HOLE/SEEK_DATA so everyone > is on the same page... We should not tie in the definition to existing fs technologies. Instead we should let the fs weigh the cost of providing accurate information with the possible gain in performance. Data: A range in a file that could contain something other than nulls. If in doubt, it is data. Hole: A range in a file that only contains nulls. ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-31 1:17 ` Sunil Mushran 0 siblings, 0 replies; 74+ messages in thread From: Sunil Mushran @ 2011-08-31 1:17 UTC (permalink / raw) To: Dave Chinner Cc: Andreas Dilger, linux-kernel, xfs, Christoph Hellwig, Josef Bacik, dchinner, linux-fsdevel, linux-btrfs, viro On 08/25/2011 06:35 PM, Dave Chinner wrote: > Agreed, that's the way I'd interpret it, too. So perhaps we need to > ensure that this interpretation is actually tested by this test? > > How about some definitions to work by: > > Data: a range of the file that contains valid data, regardless of > whether it exists in memory or on disk. The valid data can be > preceeded and/or followed by an arbitrary number of zero bytes > dependent on the underlying implementation of hole detection. > > Hole: a range of the file that contains no data or is made up > entirely of NULL (zero) data. Holes include preallocated ranges of > files that have not had actual data written to them. > > Does that make sense? It has sufficient flexibility in it for the > existing generic "non-implementation", allows for filesystems to > define their own hole detection boundaries (e.g. filesystem block > size), and effectively defines how preallocated ranges from > fallocate() should be treated (i.e. as holes). If we can agree on > those definitions, I think that we should document them in both the > kernel and the man page that defines SEEK_HOLE/SEEK_DATA so everyone > is on the same page... We should not tie in the definition to existing fs technologies. Instead we should let the fs weigh the cost of providing accurate information with the possible gain in performance. Data: A range in a file that could contain something other than nulls. If in doubt, it is data. Hole: A range in a file that only contains nulls. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-08-31 1:17 ` Sunil Mushran @ 2011-08-31 3:29 ` Dave Chinner -1 siblings, 0 replies; 74+ messages in thread From: Dave Chinner @ 2011-08-31 3:29 UTC (permalink / raw) To: Sunil Mushran Cc: Andreas Dilger, Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner On Tue, Aug 30, 2011 at 06:17:02PM -0700, Sunil Mushran wrote: > On 08/25/2011 06:35 PM, Dave Chinner wrote: > >Agreed, that's the way I'd interpret it, too. So perhaps we need to > >ensure that this interpretation is actually tested by this test? > > > >How about some definitions to work by: > > > >Data: a range of the file that contains valid data, regardless of > >whether it exists in memory or on disk. The valid data can be > >preceeded and/or followed by an arbitrary number of zero bytes > >dependent on the underlying implementation of hole detection. > > > >Hole: a range of the file that contains no data or is made up > >entirely of NULL (zero) data. Holes include preallocated ranges of > >files that have not had actual data written to them. > > > >Does that make sense? It has sufficient flexibility in it for the > >existing generic "non-implementation", allows for filesystems to > >define their own hole detection boundaries (e.g. filesystem block > >size), and effectively defines how preallocated ranges from > >fallocate() should be treated (i.e. as holes). If we can agree on > >those definitions, I think that we should document them in both the > >kernel and the man page that defines SEEK_HOLE/SEEK_DATA so everyone > >is on the same page... > > We should not tie in the definition to existing fs technologies. Such as? If we don't use well known, well defined terminology, we end up with ambiguous, vague functionality and inconsistent implementations. > Instead > we should let the fs weigh the cost of providing accurate information > with the possible gain in performance. > > Data: > A range in a file that could contain something other than nulls. > If in doubt, it is data. > > Hole: > A range in a file that only contains nulls. And that's -exactly- the ambiguous, vague definition that has raised all these questions in the first place. I was in doubt about whether unwritten extents can be considered a hole, and by your definition that means it should be data. But Andreas seems to be in no doubt it should be considered a hole. Hence if I implement XFS support and Andreas implements ext4 support by your defintion, we end with vastly different behaviour even though the two filesystems use the same underlying technology for preallocated ranges. That's exactly the inconsistency in implementation that I'd like us to avoid. IOWs, the definition needs to be clear enough to prevent these inconsistencies from occurring. Indeed, the phrase "preallocated ranges that have not had data written to them" is as independent of filesystem implementation or technologies as possible. However, because Linux supports preallocation (unlike our reference platform), and we encourage developers to use it where appropriate, it is best that we define how we expect such ranges to behave clearly. That makes life easier for everyone. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-31 3:29 ` Dave Chinner 0 siblings, 0 replies; 74+ messages in thread From: Dave Chinner @ 2011-08-31 3:29 UTC (permalink / raw) To: Sunil Mushran Cc: Andreas Dilger, linux-kernel, xfs, Christoph Hellwig, Josef Bacik, dchinner, linux-fsdevel, linux-btrfs, viro On Tue, Aug 30, 2011 at 06:17:02PM -0700, Sunil Mushran wrote: > On 08/25/2011 06:35 PM, Dave Chinner wrote: > >Agreed, that's the way I'd interpret it, too. So perhaps we need to > >ensure that this interpretation is actually tested by this test? > > > >How about some definitions to work by: > > > >Data: a range of the file that contains valid data, regardless of > >whether it exists in memory or on disk. The valid data can be > >preceeded and/or followed by an arbitrary number of zero bytes > >dependent on the underlying implementation of hole detection. > > > >Hole: a range of the file that contains no data or is made up > >entirely of NULL (zero) data. Holes include preallocated ranges of > >files that have not had actual data written to them. > > > >Does that make sense? It has sufficient flexibility in it for the > >existing generic "non-implementation", allows for filesystems to > >define their own hole detection boundaries (e.g. filesystem block > >size), and effectively defines how preallocated ranges from > >fallocate() should be treated (i.e. as holes). If we can agree on > >those definitions, I think that we should document them in both the > >kernel and the man page that defines SEEK_HOLE/SEEK_DATA so everyone > >is on the same page... > > We should not tie in the definition to existing fs technologies. Such as? If we don't use well known, well defined terminology, we end up with ambiguous, vague functionality and inconsistent implementations. > Instead > we should let the fs weigh the cost of providing accurate information > with the possible gain in performance. > > Data: > A range in a file that could contain something other than nulls. > If in doubt, it is data. > > Hole: > A range in a file that only contains nulls. And that's -exactly- the ambiguous, vague definition that has raised all these questions in the first place. I was in doubt about whether unwritten extents can be considered a hole, and by your definition that means it should be data. But Andreas seems to be in no doubt it should be considered a hole. Hence if I implement XFS support and Andreas implements ext4 support by your defintion, we end with vastly different behaviour even though the two filesystems use the same underlying technology for preallocated ranges. That's exactly the inconsistency in implementation that I'd like us to avoid. IOWs, the definition needs to be clear enough to prevent these inconsistencies from occurring. Indeed, the phrase "preallocated ranges that have not had data written to them" is as independent of filesystem implementation or technologies as possible. However, because Linux supports preallocation (unlike our reference platform), and we encourage developers to use it where appropriate, it is best that we define how we expect such ranges to behave clearly. That makes life easier for everyone. Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-08-31 3:29 ` Dave Chinner @ 2011-08-31 3:53 ` david -1 siblings, 0 replies; 74+ messages in thread From: david @ 2011-08-31 3:53 UTC (permalink / raw) To: Dave Chinner Cc: Sunil Mushran, Andreas Dilger, Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner On Wed, 31 Aug 2011, Dave Chinner wrote: > On Tue, Aug 30, 2011 at 06:17:02PM -0700, Sunil Mushran wrote: >> On 08/25/2011 06:35 PM, Dave Chinner wrote: >>> Agreed, that's the way I'd interpret it, too. So perhaps we need to >>> ensure that this interpretation is actually tested by this test? >>> >>> How about some definitions to work by: >>> >>> Data: a range of the file that contains valid data, regardless of >>> whether it exists in memory or on disk. The valid data can be >>> preceeded and/or followed by an arbitrary number of zero bytes >>> dependent on the underlying implementation of hole detection. >>> >>> Hole: a range of the file that contains no data or is made up >>> entirely of NULL (zero) data. Holes include preallocated ranges of >>> files that have not had actual data written to them. >>> >>> Does that make sense? It has sufficient flexibility in it for the >>> existing generic "non-implementation", allows for filesystems to >>> define their own hole detection boundaries (e.g. filesystem block >>> size), and effectively defines how preallocated ranges from >>> fallocate() should be treated (i.e. as holes). If we can agree on >>> those definitions, I think that we should document them in both the >>> kernel and the man page that defines SEEK_HOLE/SEEK_DATA so everyone >>> is on the same page... >> >> We should not tie in the definition to existing fs technologies. > > Such as? If we don't use well known, well defined terminology, we > end up with ambiguous, vague functionality and inconsistent > implementations. > >> Instead >> we should let the fs weigh the cost of providing accurate information >> with the possible gain in performance. >> >> Data: >> A range in a file that could contain something other than nulls. >> If in doubt, it is data. >> >> Hole: >> A range in a file that only contains nulls. > > And that's -exactly- the ambiguous, vague definition that has raised > all these questions in the first place. I was in doubt about whether > unwritten extents can be considered a hole, and by your definition > that means it should be data. But Andreas seems to be in no doubt it > should be considered a hole. > > Hence if I implement XFS support and Andreas implements ext4 support > by your defintion, we end with vastly different behaviour even > though the two filesystems use the same underlying technology for > preallocated ranges. That's exactly the inconsistency in > implementation that I'd like us to avoid. > > IOWs, the definition needs to be clear enough to prevent these > inconsistencies from occurring. Indeed, the phrase "preallocated > ranges that have not had data written to them" is as independent of > filesystem implementation or technologies as possible. However, > because Linux supports preallocation (unlike our reference > platform), and we encourage developers to use it where appropriate, > it is best that we define how we expect such ranges to behave > clearly. That makes life easier for everyone. Since a sparse file has the holes filled by nulls by definition, it seems fairly clear that they chould count as holes. In fact, I would not be surprised to see some filesystem _only_ report the unwritten pieces of sparse files as holes (not any other ranges of nulls) the question I have is how large does the range of nulls need to be before it's reported as a hole? disk sectors, filesystem blocks, other? David Lang ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-31 3:53 ` david 0 siblings, 0 replies; 74+ messages in thread From: david @ 2011-08-31 3:53 UTC (permalink / raw) To: Dave Chinner Cc: Sunil Mushran, Andreas Dilger, linux-kernel, xfs, Christoph Hellwig, linux-btrfs, dchinner, linux-fsdevel, Josef Bacik, viro On Wed, 31 Aug 2011, Dave Chinner wrote: > On Tue, Aug 30, 2011 at 06:17:02PM -0700, Sunil Mushran wrote: >> On 08/25/2011 06:35 PM, Dave Chinner wrote: >>> Agreed, that's the way I'd interpret it, too. So perhaps we need to >>> ensure that this interpretation is actually tested by this test? >>> >>> How about some definitions to work by: >>> >>> Data: a range of the file that contains valid data, regardless of >>> whether it exists in memory or on disk. The valid data can be >>> preceeded and/or followed by an arbitrary number of zero bytes >>> dependent on the underlying implementation of hole detection. >>> >>> Hole: a range of the file that contains no data or is made up >>> entirely of NULL (zero) data. Holes include preallocated ranges of >>> files that have not had actual data written to them. >>> >>> Does that make sense? It has sufficient flexibility in it for the >>> existing generic "non-implementation", allows for filesystems to >>> define their own hole detection boundaries (e.g. filesystem block >>> size), and effectively defines how preallocated ranges from >>> fallocate() should be treated (i.e. as holes). If we can agree on >>> those definitions, I think that we should document them in both the >>> kernel and the man page that defines SEEK_HOLE/SEEK_DATA so everyone >>> is on the same page... >> >> We should not tie in the definition to existing fs technologies. > > Such as? If we don't use well known, well defined terminology, we > end up with ambiguous, vague functionality and inconsistent > implementations. > >> Instead >> we should let the fs weigh the cost of providing accurate information >> with the possible gain in performance. >> >> Data: >> A range in a file that could contain something other than nulls. >> If in doubt, it is data. >> >> Hole: >> A range in a file that only contains nulls. > > And that's -exactly- the ambiguous, vague definition that has raised > all these questions in the first place. I was in doubt about whether > unwritten extents can be considered a hole, and by your definition > that means it should be data. But Andreas seems to be in no doubt it > should be considered a hole. > > Hence if I implement XFS support and Andreas implements ext4 support > by your defintion, we end with vastly different behaviour even > though the two filesystems use the same underlying technology for > preallocated ranges. That's exactly the inconsistency in > implementation that I'd like us to avoid. > > IOWs, the definition needs to be clear enough to prevent these > inconsistencies from occurring. Indeed, the phrase "preallocated > ranges that have not had data written to them" is as independent of > filesystem implementation or technologies as possible. However, > because Linux supports preallocation (unlike our reference > platform), and we encourage developers to use it where appropriate, > it is best that we define how we expect such ranges to behave > clearly. That makes life easier for everyone. Since a sparse file has the holes filled by nulls by definition, it seems fairly clear that they chould count as holes. In fact, I would not be surprised to see some filesystem _only_ report the unwritten pieces of sparse files as holes (not any other ranges of nulls) the question I have is how large does the range of nulls need to be before it's reported as a hole? disk sectors, filesystem blocks, other? David Lang _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-08-31 3:29 ` Dave Chinner @ 2011-08-31 4:43 ` Sunil Mushran -1 siblings, 0 replies; 74+ messages in thread From: Sunil Mushran @ 2011-08-31 4:43 UTC (permalink / raw) To: Dave Chinner Cc: Andreas Dilger, Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner On 8/30/2011 8:29 PM, Dave Chinner wrote: > And that's -exactly- the ambiguous, vague definition that has raised > all these questions in the first place. I was in doubt about whether > unwritten extents can be considered a hole, and by your definition > that means it should be data. But Andreas seems to be in no doubt it > should be considered a hole. Fair enough. Let me rephrase. Data: A range in a file when read could return something other than nulls. Hole: A range in a file when read only returns nulls. Considering preallocated extents only return null, they should be considered holes. ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-31 4:43 ` Sunil Mushran 0 siblings, 0 replies; 74+ messages in thread From: Sunil Mushran @ 2011-08-31 4:43 UTC (permalink / raw) To: Dave Chinner Cc: Andreas Dilger, linux-kernel, xfs, Christoph Hellwig, Josef Bacik, dchinner, linux-fsdevel, linux-btrfs, viro On 8/30/2011 8:29 PM, Dave Chinner wrote: > And that's -exactly- the ambiguous, vague definition that has raised > all these questions in the first place. I was in doubt about whether > unwritten extents can be considered a hole, and by your definition > that means it should be data. But Andreas seems to be in no doubt it > should be considered a hole. Fair enough. Let me rephrase. Data: A range in a file when read could return something other than nulls. Hole: A range in a file when read only returns nulls. Considering preallocated extents only return null, they should be considered holes. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-08-31 4:43 ` Sunil Mushran (?) @ 2011-08-31 9:05 ` Pádraig Brady -1 siblings, 0 replies; 74+ messages in thread From: Pádraig Brady @ 2011-08-31 9:05 UTC (permalink / raw) To: Sunil Mushran Cc: Dave Chinner, Andreas Dilger, Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner On 08/31/2011 05:43 AM, Sunil Mushran wrote: > On 8/30/2011 8:29 PM, Dave Chinner wrote: >> And that's -exactly- the ambiguous, vague definition that has raised >> all these questions in the first place. I was in doubt about whether >> unwritten extents can be considered a hole, and by your definition >> that means it should be data. But Andreas seems to be in no doubt it >> should be considered a hole. >=20 > Fair enough. Let me rephrase. >=20 > Data: > A range in a file when read could return something other than nulls. >=20 > Hole: > A range in a file when read only returns nulls. >=20 > Considering preallocated extents only return null, they should > be considered holes. I would concur with this. Treating preallocated extents as holes will allow them to be processed quickly by `cp` etc. What we lose is the ability to copy the allocation from src to dst. But that's less important in comparison, and could probably be done on a per file rather than per extent basis anyway. Note preallocation would be good for disk layout and timely ENOSPC. Note fiemap gives us greater control by distinguishing holes and empty extents, thus allowing us to both efficiently copy and maintain allocation. But that interface currently requires a sync of the file on some file systems, which we couldn't enable for performance reasons in cp. cheers, P=E1draig. ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-31 9:05 ` Pádraig Brady 0 siblings, 0 replies; 74+ messages in thread From: Pádraig Brady @ 2011-08-31 9:05 UTC (permalink / raw) To: Sunil Mushran Cc: Andreas Dilger, linux-kernel, xfs, Christoph Hellwig, linux-btrfs, dchinner, linux-fsdevel, Josef Bacik, viro On 08/31/2011 05:43 AM, Sunil Mushran wrote: > On 8/30/2011 8:29 PM, Dave Chinner wrote: >> And that's -exactly- the ambiguous, vague definition that has raised >> all these questions in the first place. I was in doubt about whether >> unwritten extents can be considered a hole, and by your definition >> that means it should be data. But Andreas seems to be in no doubt it >> should be considered a hole. > > Fair enough. Let me rephrase. > > Data: > A range in a file when read could return something other than nulls. > > Hole: > A range in a file when read only returns nulls. > > Considering preallocated extents only return null, they should > be considered holes. I would concur with this. Treating preallocated extents as holes will allow them to be processed quickly by `cp` etc. What we lose is the ability to copy the allocation from src to dst. But that's less important in comparison, and could probably be done on a per file rather than per extent basis anyway. Note preallocation would be good for disk layout and timely ENOSPC. Note fiemap gives us greater control by distinguishing holes and empty extents, thus allowing us to both efficiently copy and maintain allocation. But that interface currently requires a sync of the file on some file systems, which we couldn't enable for performance reasons in cp. cheers, Pádraig. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-31 9:05 ` Pádraig Brady 0 siblings, 0 replies; 74+ messages in thread From: Pádraig Brady @ 2011-08-31 9:05 UTC (permalink / raw) To: Sunil Mushran Cc: Dave Chinner, Andreas Dilger, Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner On 08/31/2011 05:43 AM, Sunil Mushran wrote: > On 8/30/2011 8:29 PM, Dave Chinner wrote: >> And that's -exactly- the ambiguous, vague definition that has raised >> all these questions in the first place. I was in doubt about whether >> unwritten extents can be considered a hole, and by your definition >> that means it should be data. But Andreas seems to be in no doubt it >> should be considered a hole. > > Fair enough. Let me rephrase. > > Data: > A range in a file when read could return something other than nulls. > > Hole: > A range in a file when read only returns nulls. > > Considering preallocated extents only return null, they should > be considered holes. I would concur with this. Treating preallocated extents as holes will allow them to be processed quickly by `cp` etc. What we lose is the ability to copy the allocation from src to dst. But that's less important in comparison, and could probably be done on a per file rather than per extent basis anyway. Note preallocation would be good for disk layout and timely ENOSPC. Note fiemap gives us greater control by distinguishing holes and empty extents, thus allowing us to both efficiently copy and maintain allocation. But that interface currently requires a sync of the file on some file systems, which we couldn't enable for performance reasons in cp. cheers, Pádraig. ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester 2011-08-31 3:29 ` Dave Chinner @ 2011-08-31 4:48 ` Dan Merillat -1 siblings, 0 replies; 74+ messages in thread From: Dan Merillat @ 2011-08-31 4:48 UTC (permalink / raw) To: Dave Chinner Cc: Sunil Mushran, Andreas Dilger, Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-kernel, linux-btrfs, xfs, viro, dchinner On Tue, Aug 30, 2011 at 11:29 PM, Dave Chinner <david@fromorbit.com> wrote: > On Tue, Aug 30, 2011 at 06:17:02PM -0700, Sunil Mushran wrote: >> Instead >> we should let the fs weigh the cost of providing accurate information >> with the possible gain in performance. >> >> Data: >> A range in a file that could contain something other than nulls. >> If in doubt, it is data. >> >> Hole: >> A range in a file that only contains nulls. > > And that's -exactly- the ambiguous, vague definition that has raised > all these questions in the first place. I was in doubt about whether > unwritten extents can be considered a hole, and by your definition > that means it should be data. But Andreas seems to be in no doubt it > should be considered a hole. That's fine, though. Different filesystems have different abilities to recognize a data hole - FAT can't do it at all. Perhaps the requirements would be better stated in reverse: If the filesystem knows that a read() will return nulls (for whatever reason based on it's internal knowledge), it can report a hole. If it can't guarantee that, it's data. It's an absolute requirement that SEEK_DATA never miss data. SEEK_HOLE working is a nicety that userspace would appreciate - remember that the consumer here is cp(1), using it to skip empty portions of files and create sparse destination files. ^ permalink raw reply [flat|nested] 74+ messages in thread
* Re: [PATCH] xfstests 255: add a seek_data/seek_hole tester @ 2011-08-31 4:48 ` Dan Merillat 0 siblings, 0 replies; 74+ messages in thread From: Dan Merillat @ 2011-08-31 4:48 UTC (permalink / raw) To: Dave Chinner Cc: Sunil Mushran, Andreas Dilger, linux-kernel, xfs, Christoph Hellwig, linux-btrfs, dchinner, linux-fsdevel, Josef Bacik, viro On Tue, Aug 30, 2011 at 11:29 PM, Dave Chinner <david@fromorbit.com> wrote: > On Tue, Aug 30, 2011 at 06:17:02PM -0700, Sunil Mushran wrote: >> Instead >> we should let the fs weigh the cost of providing accurate information >> with the possible gain in performance. >> >> Data: >> A range in a file that could contain something other than nulls. >> If in doubt, it is data. >> >> Hole: >> A range in a file that only contains nulls. > > And that's -exactly- the ambiguous, vague definition that has raised > all these questions in the first place. I was in doubt about whether > unwritten extents can be considered a hole, and by your definition > that means it should be data. But Andreas seems to be in no doubt it > should be considered a hole. That's fine, though. Different filesystems have different abilities to recognize a data hole - FAT can't do it at all. Perhaps the requirements would be better stated in reverse: If the filesystem knows that a read() will return nulls (for whatever reason based on it's internal knowledge), it can report a hole. If it can't guarantee that, it's data. It's an absolute requirement that SEEK_DATA never miss data. SEEK_HOLE working is a nicety that userspace would appreciate - remember that the consumer here is cp(1), using it to skip empty portions of files and create sparse destination files. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 74+ messages in thread
end of thread, other threads:[~2011-08-31 9:05 UTC | newest] Thread overview: 74+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-06-27 18:02 [PATCH 1/4] fs: add SEEK_HOLE and SEEK_DATA flags Josef Bacik 2011-06-27 18:02 ` [PATCH 2/4] Btrfs: implement our own ->llseek Josef Bacik 2011-06-27 18:02 ` [PATCH 3/4] Ext4: handle SEEK_HOLE/SEEK_DATA generically Josef Bacik 2011-06-27 18:02 ` [PATCH 4/4] fs: handle SEEK_HOLE/SEEK_DATA properly in all fs's that define their own llseek Josef Bacik 2011-06-27 18:02 ` [PATCH] xfstests 255: add a seek_data/seek_hole tester Josef Bacik 2011-06-27 18:32 ` Andreas Dilger 2011-06-27 18:47 ` Josef Bacik 2011-06-27 21:04 ` [PATCH 1/4] fs: add SEEK_HOLE and SEEK_DATA flags Josef Bacik 2011-06-28 15:33 Josef Bacik 2011-06-28 15:33 ` [PATCH] xfstests 255: add a seek_data/seek_hole tester Josef Bacik 2011-06-28 15:33 ` Josef Bacik 2011-06-29 6:53 ` Dave Chinner 2011-06-29 6:53 ` Dave Chinner 2011-06-29 6:53 ` Dave Chinner 2011-06-29 6:53 ` Dave Chinner 2011-06-29 7:40 ` Christoph Hellwig 2011-06-29 7:40 ` Christoph Hellwig 2011-06-29 10:42 ` Pádraig Brady 2011-06-29 10:42 ` Pádraig Brady 2011-06-29 10:42 ` Pádraig Brady 2011-06-29 10:42 ` Pádraig Brady 2011-06-29 17:29 ` Sunil Mushran 2011-06-29 17:29 ` Sunil Mushran 2011-06-29 17:29 ` Sunil Mushran 2011-06-29 17:29 ` Sunil Mushran 2011-06-29 17:36 ` Christoph Hellwig 2011-06-29 17:36 ` Christoph Hellwig 2011-06-29 17:40 ` Sunil Mushran 2011-06-29 17:40 ` Sunil Mushran 2011-06-29 21:29 ` Pádraig Brady 2011-06-29 21:29 ` Pádraig Brady 2011-06-29 21:29 ` Pádraig Brady 2011-07-01 9:37 ` Christoph Hellwig 2011-07-01 9:37 ` Christoph Hellwig 2011-06-29 17:10 ` Sunil Mushran 2011-06-29 17:10 ` Sunil Mushran 2011-06-29 17:52 ` Josef Bacik 2011-06-29 17:52 ` Josef Bacik 2011-06-29 13:19 ` Josef Bacik 2011-06-29 13:19 ` Josef Bacik 2011-06-29 13:19 ` Josef Bacik 2011-08-25 6:06 ` Christoph Hellwig 2011-08-25 6:06 ` Christoph Hellwig 2011-08-25 6:40 ` Dave Chinner 2011-08-25 6:40 ` Dave Chinner 2011-08-25 6:51 ` Andreas Dilger 2011-08-25 6:51 ` Andreas Dilger 2011-08-26 1:35 ` Dave Chinner 2011-08-26 1:35 ` Dave Chinner 2011-08-26 6:24 ` Marco Stornelli 2011-08-26 6:24 ` Marco Stornelli 2011-08-26 6:24 ` Marco Stornelli 2011-08-26 14:41 ` Zach Brown 2011-08-26 14:41 ` Zach Brown 2011-08-26 14:41 ` Zach Brown 2011-08-26 14:41 ` Zach Brown 2011-08-27 8:30 ` Marco Stornelli 2011-08-27 8:30 ` Marco Stornelli 2011-08-28 10:17 ` Marco Stornelli 2011-08-28 10:17 ` Marco Stornelli 2011-08-30 17:42 ` Sunil Mushran 2011-08-30 17:42 ` Sunil Mushran 2011-08-31 1:17 ` Sunil Mushran 2011-08-31 1:17 ` Sunil Mushran 2011-08-31 3:29 ` Dave Chinner 2011-08-31 3:29 ` Dave Chinner 2011-08-31 3:53 ` david 2011-08-31 3:53 ` david 2011-08-31 4:43 ` Sunil Mushran 2011-08-31 4:43 ` Sunil Mushran 2011-08-31 9:05 ` Pádraig Brady 2011-08-31 9:05 ` Pádraig Brady 2011-08-31 9:05 ` Pádraig Brady 2011-08-31 4:48 ` Dan Merillat 2011-08-31 4:48 ` Dan Merillat
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.