* [patch 1/6][RFC] Keep FIBMAP from looking at negative block nrs
2007-10-26 23:37 [patch 0/6][RFC] Cleanup FIBMAP Mike Waychison
@ 2007-10-26 23:37 ` Mike Waychison
2007-10-26 23:37 ` [patch 2/6][RFC] Allow FIBMAP to return EFBIG on large filesystems Mike Waychison
` (6 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Mike Waychison @ 2007-10-26 23:37 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-kernel, Mike Waychison
[-- Attachment #1: fix_negative_fibmap_blocks.patch --]
[-- Type: text/plain, Size: 665 bytes --]
Return an error if the user requests a negative logical block in a file.
Signed-off-by: Mike Waychison <mikew@google.com>
fs/ioctl.c | 2 ++
1 file changed, 2 insertions(+)
Index: linux-2.6.23/fs/ioctl.c
===================================================================
--- linux-2.6.23.orig/fs/ioctl.c 2007-10-26 15:25:48.000000000 -0700
+++ linux-2.6.23/fs/ioctl.c 2007-10-26 16:16:29.000000000 -0700
@@ -60,6 +60,8 @@ static int file_ioctl(struct file *filp,
return -EPERM;
if ((error = get_user(block, p)) != 0)
return error;
+ if (block < 0)
+ return -EINVAL;
lock_kernel();
res = mapping->a_ops->bmap(mapping, block);
--
^ permalink raw reply [flat|nested] 23+ messages in thread* [patch 2/6][RFC] Allow FIBMAP to return EFBIG on large filesystems.
2007-10-26 23:37 [patch 0/6][RFC] Cleanup FIBMAP Mike Waychison
2007-10-26 23:37 ` [patch 1/6][RFC] Keep FIBMAP from looking at negative block nrs Mike Waychison
@ 2007-10-26 23:37 ` Mike Waychison
2007-10-26 23:37 ` [patch 3/6][RFC] Move FIBMAP logic Mike Waychison
` (5 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Mike Waychison @ 2007-10-26 23:37 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-kernel, Mike Waychison
[-- Attachment #1: add_fibmap_check_32bit.patch --]
[-- Type: text/plain, Size: 1167 bytes --]
Propagate an error (EFBIG) to userspace if the physical block is too large to return in a 32bit int instead of truncating it.
Signed-off-by: Mike Waychison <mikew@google.com>
fs/ioctl.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
Index: linux-2.6.23/fs/ioctl.c
===================================================================
--- linux-2.6.23.orig/fs/ioctl.c 2007-10-26 15:26:10.000000000 -0700
+++ linux-2.6.23/fs/ioctl.c 2007-10-26 16:16:28.000000000 -0700
@@ -52,6 +52,7 @@ static int file_ioctl(struct file *filp,
case FIBMAP:
{
struct address_space *mapping = filp->f_mapping;
+ sector_t phys_block;
int res;
/* do we support this mess? */
if (!mapping->a_ops->bmap)
@@ -64,8 +65,15 @@ static int file_ioctl(struct file *filp,
return -EINVAL;
lock_kernel();
- res = mapping->a_ops->bmap(mapping, block);
+ phys_block = mapping->a_ops->bmap(mapping, block);
unlock_kernel();
+
+ /* Make sure that the return value fits in the
+ * user's buffer. */
+ if ((u32)phys_block < phys_block)
+ return -EFBIG;
+
+ res = phys_block;
return put_user(res, p);
}
case FIGETBSZ:
--
^ permalink raw reply [flat|nested] 23+ messages in thread* [patch 3/6][RFC] Move FIBMAP logic
2007-10-26 23:37 [patch 0/6][RFC] Cleanup FIBMAP Mike Waychison
2007-10-26 23:37 ` [patch 1/6][RFC] Keep FIBMAP from looking at negative block nrs Mike Waychison
2007-10-26 23:37 ` [patch 2/6][RFC] Allow FIBMAP to return EFBIG on large filesystems Mike Waychison
@ 2007-10-26 23:37 ` Mike Waychison
2007-10-26 23:37 ` [patch 4/6][RFC] Attempt to plug race with truncate Mike Waychison
` (4 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Mike Waychison @ 2007-10-26 23:37 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-kernel, Mike Waychison
[-- Attachment #1: move_fibmap_logic.patch --]
[-- Type: text/plain, Size: 1539 bytes --]
Move FIBMAP logic out of file_ioctl() in preparation for introducing FIBMAP64.
Signed-off-by: Mike Waychison <mikew@google.com>
fs/ioctl.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
Index: linux-2.6.23/fs/ioctl.c
===================================================================
--- linux-2.6.23.orig/fs/ioctl.c 2007-10-26 15:26:11.000000000 -0700
+++ linux-2.6.23/fs/ioctl.c 2007-10-26 16:16:28.000000000 -0700
@@ -40,6 +40,21 @@ static long do_ioctl(struct file *filp,
return error;
}
+static int do_fibmap(struct address_space *mapping, sector_t block,
+ sector_t *phys_block)
+{
+ if (!capable(CAP_SYS_RAWIO))
+ return -EPERM;
+ if (!mapping->a_ops->bmap)
+ return -EINVAL;
+
+ lock_kernel();
+ *phys_block = mapping->a_ops->bmap(mapping, block);
+ unlock_kernel();
+
+ return 0;
+}
+
static int file_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg)
{
@@ -55,18 +70,14 @@ static int file_ioctl(struct file *filp,
sector_t phys_block;
int res;
/* do we support this mess? */
- if (!mapping->a_ops->bmap)
- return -EINVAL;
- if (!capable(CAP_SYS_RAWIO))
- return -EPERM;
if ((error = get_user(block, p)) != 0)
return error;
if (block < 0)
return -EINVAL;
- lock_kernel();
- phys_block = mapping->a_ops->bmap(mapping, block);
- unlock_kernel();
+ error = do_fibmap(mapping, block, &phys_block);
+ if (error)
+ return error;
/* Make sure that the return value fits in the
* user's buffer. */
--
^ permalink raw reply [flat|nested] 23+ messages in thread* [patch 4/6][RFC] Attempt to plug race with truncate
2007-10-26 23:37 [patch 0/6][RFC] Cleanup FIBMAP Mike Waychison
` (2 preceding siblings ...)
2007-10-26 23:37 ` [patch 3/6][RFC] Move FIBMAP logic Mike Waychison
@ 2007-10-26 23:37 ` Mike Waychison
2007-10-29 13:36 ` Chris Mason
2007-10-26 23:37 ` [patch 5/6][RFC] Introduce FIBMAP64 Mike Waychison
` (3 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Mike Waychison @ 2007-10-26 23:37 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-kernel, Mike Waychison
[-- Attachment #1: fix_race_with_truncate.patch --]
[-- Type: text/plain, Size: 1161 bytes --]
Attempt to deal with races with truncate paths.
I'm not really sure on the locking here, but these seem to be taken by the truncate path. BKL is left as some filesystem may(?) still require it.
Signed-off-by: Mike Waychison <mikew@google.com>
fs/ioctl.c | 8 ++++++++
1 file changed, 8 insertions(+)
Index: linux-2.6.23/fs/ioctl.c
===================================================================
--- linux-2.6.23.orig/fs/ioctl.c 2007-10-26 15:27:29.000000000 -0700
+++ linux-2.6.23/fs/ioctl.c 2007-10-26 16:16:28.000000000 -0700
@@ -43,13 +43,21 @@ static long do_ioctl(struct file *filp,
static int do_fibmap(struct address_space *mapping, sector_t block,
sector_t *phys_block)
{
+ struct inode *inode = mapping->host;
+
if (!capable(CAP_SYS_RAWIO))
return -EPERM;
if (!mapping->a_ops->bmap)
return -EINVAL;
lock_kernel();
+ /* Avoid races with truncate */
+ mutex_lock(&inode->i_mutex);
+ /* FIXME: Do we really need i_alloc_sem? */
+ down_read(&inode->i_alloc_sem);
*phys_block = mapping->a_ops->bmap(mapping, block);
+ up_read(&inode->i_alloc_sem);
+ mutex_unlock(&inode->i_mutex);
unlock_kernel();
return 0;
--
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [patch 4/6][RFC] Attempt to plug race with truncate
2007-10-26 23:37 ` [patch 4/6][RFC] Attempt to plug race with truncate Mike Waychison
@ 2007-10-29 13:36 ` Chris Mason
0 siblings, 0 replies; 23+ messages in thread
From: Chris Mason @ 2007-10-29 13:36 UTC (permalink / raw)
To: Mike Waychison; +Cc: linux-fsdevel, linux-kernel, Mike Waychison
On Fri, 26 Oct 2007 16:37:36 -0700
Mike Waychison <mikew@google.com> wrote:
> Attempt to deal with races with truncate paths.
>
> I'm not really sure on the locking here, but these seem to be taken
> by the truncate path. BKL is left as some filesystem may(?) still
> require it.
>
> Signed-off-by: Mike Waychison <mikew@google.com>
> fs/ioctl.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> Index: linux-2.6.23/fs/ioctl.c
> ===================================================================
> --- linux-2.6.23.orig/fs/ioctl.c 2007-10-26 15:27:29.000000000
> -0700 +++ linux-2.6.23/fs/ioctl.c 2007-10-26
> 16:16:28.000000000 -0700 @@ -43,13 +43,21 @@ static long
> do_ioctl(struct file *filp, static int do_fibmap(struct address_space
> *mapping, sector_t block, sector_t *phys_block)
> {
> + struct inode *inode = mapping->host;
> +
> if (!capable(CAP_SYS_RAWIO))
> return -EPERM;
> if (!mapping->a_ops->bmap)
> return -EINVAL;
>
> lock_kernel();
> + /* Avoid races with truncate */
> + mutex_lock(&inode->i_mutex);
> + /* FIXME: Do we really need i_alloc_sem? */
> + down_read(&inode->i_alloc_sem);
i_alloc_sem will avoid races with filesystems filling holes inside
writepage (where i_mutex isn't held). I'd expect everyone to currently
give some consistent result (either the old value or the new but not
garbage), but I wouldn't expect taking the semaphore to hurt anything.
-chris
^ permalink raw reply [flat|nested] 23+ messages in thread
* [patch 5/6][RFC] Introduce FIBMAP64
2007-10-26 23:37 [patch 0/6][RFC] Cleanup FIBMAP Mike Waychison
` (3 preceding siblings ...)
2007-10-26 23:37 ` [patch 4/6][RFC] Attempt to plug race with truncate Mike Waychison
@ 2007-10-26 23:37 ` Mike Waychison
2007-10-29 13:45 ` Chris Mason
2007-10-26 23:37 ` [patch 6/6][RFC] Drop CAP_SYS_RAWIO requirement on FIBMAP Mike Waychison
` (2 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Mike Waychison @ 2007-10-26 23:37 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-kernel, Mike Waychison
[-- Attachment #1: add_fibmap64.patch --]
[-- Type: text/plain, Size: 4090 bytes --]
Introduce FIBMAP64. This is the same as FIBMAP, but takes a u64.
This new routine requires filesystems to implement bmap64 if they want to support > 2^31 blocks in a logical file. We do this to give time for filesystems to be properly audited.
Signed-off-by: Mike Waychison <mikew@google.com>
fs/compat_ioctl.c | 2 ++
fs/ioctl.c | 34 +++++++++++++++++++++++++++++++++-
include/linux/fs.h | 7 +++++++
3 files changed, 42 insertions(+), 1 deletion(-)
Index: linux-2.6.23/fs/ioctl.c
===================================================================
--- linux-2.6.23.orig/fs/ioctl.c 2007-10-26 15:28:28.000000000 -0700
+++ linux-2.6.23/fs/ioctl.c 2007-10-26 16:16:28.000000000 -0700
@@ -44,11 +44,24 @@ static int do_fibmap(struct address_spac
sector_t *phys_block)
{
struct inode *inode = mapping->host;
+ sector_t (*bmap)(struct address_space *, sector_t);
if (!capable(CAP_SYS_RAWIO))
return -EPERM;
- if (!mapping->a_ops->bmap)
+
+ if (mapping->a_ops->bmap64) {
+ /* Filesystem has bmap path audited for 64bit. */
+ bmap = mapping->a_ops->bmap64;
+ } else if (mapping->a_ops->bmap) {
+ bmap = mapping->a_ops->bmap;
+ /* Need to verify that the input looks sane when sector_t is
+ * 64bit and the filesystem has only been audited for 32bit. */
+ if ((s32)block < 0)
+ return -EFBIG;
+ } else {
+ /* no FIBMAP support */
return -EINVAL;
+ }
lock_kernel();
/* Avoid races with truncate */
@@ -95,6 +108,25 @@ static int file_ioctl(struct file *filp,
res = phys_block;
return put_user(res, p);
}
+ case FIBMAP64:
+ {
+ struct address_space *mapping = filp->f_mapping;
+ sector_t phys_block;
+ u64 block;
+
+ if ((error = get_user(block, p)) != 0)
+ return error;
+
+ /* Make sure that the logical block fits in sector_t */
+ if ((sector_t)-1 < block)
+ return -EFBIG;
+
+ error = do_fibmap(mapping, block, &phys_block);
+ if (error)
+ return error;
+
+ return put_user(block, p);
+ }
case FIGETBSZ:
return put_user(inode->i_sb->s_blocksize, p);
case FIONREAD:
Index: linux-2.6.23/include/linux/fs.h
===================================================================
--- linux-2.6.23.orig/include/linux/fs.h 2007-10-26 15:25:47.000000000 -0700
+++ linux-2.6.23/include/linux/fs.h 2007-10-26 15:29:00.000000000 -0700
@@ -220,6 +220,7 @@ extern int dir_notify_enable;
#define BMAP_IOCTL 1 /* obsolete - kept for compatibility */
#define FIBMAP _IO(0x00,1) /* bmap access */
#define FIGETBSZ _IO(0x00,2) /* get the block size used for bmap */
+#define FIBMAP64 _IO(0x00,3) /* bmap access - 64 bit sector numbers */
#define FS_IOC_GETFLAGS _IOR('f', 1, long)
#define FS_IOC_SETFLAGS _IOW('f', 2, long)
@@ -423,6 +424,12 @@ struct address_space_operations {
int (*commit_write)(struct file *, struct page *, unsigned, unsigned);
/* Unfortunately this kludge is needed for FIBMAP. Don't use it */
sector_t (*bmap)(struct address_space *, sector_t);
+ /*
+ * Version of bmap for filesystems that can safely handle 64bit
+ * sector_t. Once all filesystems are converted to this, we can drop
+ * bmap().
+ */
+ sector_t (*bmap64)(struct address_space *, sector_t);
void (*invalidatepage) (struct page *, unsigned long);
int (*releasepage) (struct page *, gfp_t);
ssize_t (*direct_IO)(int, struct kiocb *, const struct iovec *iov,
Index: linux-2.6.23/fs/compat_ioctl.c
===================================================================
--- linux-2.6.23.orig/fs/compat_ioctl.c 2007-10-26 15:25:47.000000000 -0700
+++ linux-2.6.23/fs/compat_ioctl.c 2007-10-26 15:29:00.000000000 -0700
@@ -2506,6 +2506,7 @@ COMPATIBLE_IOCTL(FIONREAD) /* This is a
/* 0x00 */
COMPATIBLE_IOCTL(FIBMAP)
COMPATIBLE_IOCTL(FIGETBSZ)
+COMPATIBLE_IOCTL(FIBMAP64)
/* 0x03 -- HD/IDE ioctl's used by hdparm and friends.
* Some need translations, these do not.
*/
@@ -3597,6 +3598,7 @@ asmlinkage long compat_sys_ioctl(unsigne
case FIBMAP:
case FIGETBSZ:
+ case FIBMAP64:
case FIONREAD:
if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
break;
--
^ permalink raw reply [flat|nested] 23+ messages in thread* [patch 6/6][RFC] Drop CAP_SYS_RAWIO requirement on FIBMAP
2007-10-26 23:37 [patch 0/6][RFC] Cleanup FIBMAP Mike Waychison
` (4 preceding siblings ...)
2007-10-26 23:37 ` [patch 5/6][RFC] Introduce FIBMAP64 Mike Waychison
@ 2007-10-26 23:37 ` Mike Waychison
2007-10-27 17:57 ` [patch 0/6][RFC] Cleanup FIBMAP Anton Altaparmakov
2007-10-28 0:43 ` H. Peter Anvin
7 siblings, 0 replies; 23+ messages in thread
From: Mike Waychison @ 2007-10-26 23:37 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-kernel, Mike Waychison
[-- Attachment #1: drop_cap_sys_rawio_for_fibmap.patch --]
[-- Type: text/plain, Size: 940 bytes --]
Remove the need for having CAP_SYS_RAWIO when doing a FIBMAP call on an open file descriptor.
It would be nice to allow users to have permission to see where their data is landing on disk, and there really isn't a good reason to keep them from getting at this information.
Signed-off-by: Mike Waychison <mikew@google.com>
fs/ioctl.c | 3 ---
1 file changed, 3 deletions(-)
Index: linux-2.6.23/fs/ioctl.c
===================================================================
--- linux-2.6.23.orig/fs/ioctl.c 2007-10-26 15:30:05.000000000 -0700
+++ linux-2.6.23/fs/ioctl.c 2007-10-26 15:31:43.000000000 -0700
@@ -46,9 +46,6 @@ static int do_fibmap(struct address_spac
struct inode *inode = mapping->host;
sector_t (*bmap)(struct address_space *, sector_t);
- if (!capable(CAP_SYS_RAWIO))
- return -EPERM;
-
if (mapping->a_ops->bmap64) {
/* Filesystem has bmap path audited for 64bit. */
bmap = mapping->a_ops->bmap64;
--
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [patch 0/6][RFC] Cleanup FIBMAP
2007-10-26 23:37 [patch 0/6][RFC] Cleanup FIBMAP Mike Waychison
` (5 preceding siblings ...)
2007-10-26 23:37 ` [patch 6/6][RFC] Drop CAP_SYS_RAWIO requirement on FIBMAP Mike Waychison
@ 2007-10-27 17:57 ` Anton Altaparmakov
2007-10-27 21:45 ` Szabolcs Szakacsits
2007-10-29 14:10 ` Chris Mason
2007-10-28 0:43 ` H. Peter Anvin
7 siblings, 2 replies; 23+ messages in thread
From: Anton Altaparmakov @ 2007-10-27 17:57 UTC (permalink / raw)
To: Mike Waychison; +Cc: linux-fsdevel, linux-kernel
Hi,
->bmap is ugly and horrible! If you have to do this at the very least
please cause ->bmap64 to be able to return error values in case the
file system failed to get the information or indeed such information
does not exist as is the case for compressed and encrypted files for
example and also for small files that are inside the on-disk inode
(NTFS resident files and reiserfs packed tails are examples of this).
And another of my pet peeves with ->bmap is that it uses 0 to mean
"sparse" which causes a conflict on NTFS at least as block zero is
part of the $Boot system file so it is a real, valid block... NTFS
uses -1 to denote sparse blocks internally.
Best regards,
Anton
On 27 Oct 2007, at 00:37, Mike Waychison wrote:
> The following series is meant to clean up FIBMAP paths with the
> eventual goal of allowing users to be able to FIBMAP their data.
>
> I'm sending this as an RFC as I've only tested this on a x86_64
> kernel with a 32bit binary on ext2 and I've noticed a couple
> ext2_warnings already.
>
> I'm unsure of the locking in [4/6] fix_race_with_truncate.patch.
> Any help here would greatly be appreciated.
>
> The last patch, [6/6] drop_cap_sys_rawio_for_fibmap.patch, is of
> course, not to be applied until any remaining issues are fixed :)
>
> Thanks,
>
> Mike Waychison
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [patch 0/6][RFC] Cleanup FIBMAP
2007-10-27 17:57 ` [patch 0/6][RFC] Cleanup FIBMAP Anton Altaparmakov
@ 2007-10-27 21:45 ` Szabolcs Szakacsits
2007-10-29 14:10 ` Chris Mason
1 sibling, 0 replies; 23+ messages in thread
From: Szabolcs Szakacsits @ 2007-10-27 21:45 UTC (permalink / raw)
To: Anton Altaparmakov; +Cc: Mike Waychison, linux-fsdevel, linux-kernel
On Sat, 27 Oct 2007, Anton Altaparmakov wrote:
> And another of my pet peeves with ->bmap is that it uses 0 to mean "sparse"
> which causes a conflict on NTFS at least as block zero is part of the $Boot
> system file so it is a real, valid block... NTFS uses -1 to denote sparse
> blocks internally.
In practice, the meaning of 0 is file system [driver] dependent. For
example in case of NTFS-3G it means that the block is sparse or the file is
encrypted or compressed, or resident, or it's the $Boot file, or an error
happened.
Thankfully the widely used FIBMAP users (swapon and the ever less used
lilo) are only interested in the non-zero values and they report an error
if the driver returns 0 for some reason. Which is perfectly ok since both
swaping and Linux booting would fail using a sparse, encrypted, compressed,
resident, or the NTFS $Boot file.
But in real, both swap files and lilo work fine with NTFS if the needed
files were created the way these softwares expect. If not then swapon or
lilo will catch and report the file creation error.
Afair, somebody is doing (has done?) an indeed much needed, better
alternative. Bmap is legacy, thank you Mike for maintaining it.
Szaka
--
NTFS-3G Lead Developer: http://ntfs-3g.org
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch 0/6][RFC] Cleanup FIBMAP
2007-10-27 17:57 ` [patch 0/6][RFC] Cleanup FIBMAP Anton Altaparmakov
2007-10-27 21:45 ` Szabolcs Szakacsits
@ 2007-10-29 14:10 ` Chris Mason
2007-10-29 16:30 ` Zach Brown
2007-10-29 19:16 ` Mike Waychison
1 sibling, 2 replies; 23+ messages in thread
From: Chris Mason @ 2007-10-29 14:10 UTC (permalink / raw)
To: Anton Altaparmakov; +Cc: Mike Waychison, linux-fsdevel, linux-kernel
On Sat, 27 Oct 2007 18:57:06 +0100
Anton Altaparmakov <aia21@cam.ac.uk> wrote:
> Hi,
>
> ->bmap is ugly and horrible! If you have to do this at the very
> least please cause ->bmap64 to be able to return error values in case
> the file system failed to get the information or indeed such
> information does not exist as is the case for compressed and
> encrypted files for example and also for small files that are inside
> the on-disk inode (NTFS resident files and reiserfs packed tails are
> examples of this).
>
> And another of my pet peeves with ->bmap is that it uses 0 to mean
> "sparse" which causes a conflict on NTFS at least as block zero is
> part of the $Boot system file so it is a real, valid block... NTFS
> uses -1 to denote sparse blocks internally.
Reiserfs and Btrfs also use 0 to mean packed. It would be nice if there
was a way to indicate your-data-is-here-but-isn't-alone. But that's
more of a feature for the FIEMAP stuff.
-chris
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch 0/6][RFC] Cleanup FIBMAP
2007-10-29 14:10 ` Chris Mason
@ 2007-10-29 16:30 ` Zach Brown
2007-10-29 19:18 ` Mike Waychison
2007-10-29 19:16 ` Mike Waychison
1 sibling, 1 reply; 23+ messages in thread
From: Zach Brown @ 2007-10-29 16:30 UTC (permalink / raw)
To: Chris Mason
Cc: Anton Altaparmakov, Mike Waychison, linux-fsdevel, linux-kernel
>> And another of my pet peeves with ->bmap is that it uses 0 to mean
>> "sparse" which causes a conflict on NTFS at least as block zero is
>> part of the $Boot system file so it is a real, valid block... NTFS
>> uses -1 to denote sparse blocks internally.
>
> Reiserfs and Btrfs also use 0 to mean packed. It would be nice if there
> was a way to indicate your-data-is-here-but-isn't-alone. But that's
> more of a feature for the FIEMAP stuff.
And maybe we can step back and see what the callers of FIBMAP are doing
with the results they're getting.
One use is to discover the order in which to read file data that will
result in efficient IO.
If we had an interface specifically for this use case then perhaps a
sparse block would be better reported as the position of the inode
relative to other data blocks. Maybe the inode block number in ext* land.
This use is interesting to me because it can be useful for any file
system, particularly networked file systems.
- z
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch 0/6][RFC] Cleanup FIBMAP
2007-10-29 16:30 ` Zach Brown
@ 2007-10-29 19:18 ` Mike Waychison
2007-10-29 19:46 ` Chris Mason
2007-10-29 20:00 ` Zach Brown
0 siblings, 2 replies; 23+ messages in thread
From: Mike Waychison @ 2007-10-29 19:18 UTC (permalink / raw)
To: Zach Brown; +Cc: Chris Mason, Anton Altaparmakov, linux-fsdevel, linux-kernel
Zach Brown wrote:
>>> And another of my pet peeves with ->bmap is that it uses 0 to mean
>>> "sparse" which causes a conflict on NTFS at least as block zero is
>>> part of the $Boot system file so it is a real, valid block... NTFS
>>> uses -1 to denote sparse blocks internally.
>> Reiserfs and Btrfs also use 0 to mean packed. It would be nice if there
>> was a way to indicate your-data-is-here-but-isn't-alone. But that's
>> more of a feature for the FIEMAP stuff.
>
> And maybe we can step back and see what the callers of FIBMAP are doing
> with the results they're getting.
>
> One use is to discover the order in which to read file data that will
> result in efficient IO.
>
> If we had an interface specifically for this use case then perhaps a
> sparse block would be better reported as the position of the inode
> relative to other data blocks. Maybe the inode block number in ext* land.
>
Can you clarify what you mean above with an example? I don't really follow.
Thanks,
Mike Waychison
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch 0/6][RFC] Cleanup FIBMAP
2007-10-29 19:18 ` Mike Waychison
@ 2007-10-29 19:46 ` Chris Mason
2007-10-29 20:01 ` Zach Brown
2007-10-29 20:00 ` Zach Brown
1 sibling, 1 reply; 23+ messages in thread
From: Chris Mason @ 2007-10-29 19:46 UTC (permalink / raw)
To: Mike Waychison
Cc: Zach Brown, Anton Altaparmakov, linux-fsdevel, linux-kernel
On Mon, 29 Oct 2007 12:18:22 -0700
Mike Waychison <mikew@google.com> wrote:
> Zach Brown wrote:
> >>> And another of my pet peeves with ->bmap is that it uses 0 to
> >>> mean "sparse" which causes a conflict on NTFS at least as block
> >>> zero is part of the $Boot system file so it is a real, valid
> >>> block... NTFS uses -1 to denote sparse blocks internally.
> >> Reiserfs and Btrfs also use 0 to mean packed. It would be nice if
> >> there was a way to indicate your-data-is-here-but-isn't-alone.
> >> But that's more of a feature for the FIEMAP stuff.
> >
> > And maybe we can step back and see what the callers of FIBMAP are
> > doing with the results they're getting.
> >
> > One use is to discover the order in which to read file data that
> > will result in efficient IO.
> >
> > If we had an interface specifically for this use case then perhaps a
> > sparse block would be better reported as the position of the inode
> > relative to other data blocks. Maybe the inode block number in
> > ext* land.
> >
>
> Can you clarify what you mean above with an example? I don't really
> follow.
This is a larger topic of helping userland optimize access to groups of
files. For example, during a readdir if we knew the next step was to
delete all the files found, we could do one top of readahead (or even
ordering the returned values).
If we knew the next step would be to read all the files found, a
different type of readahead would be useful.
But, we shouldn't inflict all of this on fibmap/fiemap....we'll get
lost trying to make the one true interface for all operations.
For grouping operations on files, I think a read_tree syscall with
hints for what userland will do (read, stat, delete, list
filenames), and a better cookie than readdir should do it.
-chris
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch 0/6][RFC] Cleanup FIBMAP
2007-10-29 19:46 ` Chris Mason
@ 2007-10-29 20:01 ` Zach Brown
0 siblings, 0 replies; 23+ messages in thread
From: Zach Brown @ 2007-10-29 20:01 UTC (permalink / raw)
To: Chris Mason
Cc: Mike Waychison, Anton Altaparmakov, linux-fsdevel, linux-kernel
> But, we shouldn't inflict all of this on fibmap/fiemap....we'll get
> lost trying to make the one true interface for all operations.
>
> For grouping operations on files, I think a read_tree syscall with
> hints for what userland will do (read, stat, delete, list
> filenames), and a better cookie than readdir should do it.
Agreed, on both points.
- z
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch 0/6][RFC] Cleanup FIBMAP
2007-10-29 19:18 ` Mike Waychison
2007-10-29 19:46 ` Chris Mason
@ 2007-10-29 20:00 ` Zach Brown
2007-10-31 11:06 ` Ric Wheeler
1 sibling, 1 reply; 23+ messages in thread
From: Zach Brown @ 2007-10-29 20:00 UTC (permalink / raw)
To: Mike Waychison
Cc: Chris Mason, Anton Altaparmakov, linux-fsdevel, linux-kernel
> Can you clarify what you mean above with an example? I don't really
> follow.
Sure, take 'tar' as an example. It'll read files in the order that
their names are returned from directory listing. This can produce bad
IO patterns because the order in which the file names are returned
doesn't match the order of the file's blocks on disk. (htree, I'm
looking at you!)
People have noticed that tar-like loads can be sped up greatly just by
sorting the files by their inode number as returned by stat(), never
mind the file blocks themselves. One example of this is Chris Mason's
'acp'.
http://oss.oracle.com/~mason/acp/
The logical extension of that is to use FIBMAP to find the order of file
blocks on disk and then doing IO on blocks in sorted order. It'd take
work to write an app that does this reliably, sure.
In this use the application doesn't actually care what the absolute
numbers are. It cares about their ordering. File systems would be able
to chose whatever scheme they wanted for the actual values of the
results from a FIBMAP-alike as long as the sorting resulted in the right
IO patterns.
Arguing that this use is significant enough to justify an addition to
the file system API is a stretch. I'm just sharing the observation.
- z
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch 0/6][RFC] Cleanup FIBMAP
2007-10-29 20:00 ` Zach Brown
@ 2007-10-31 11:06 ` Ric Wheeler
2007-10-31 16:16 ` Zach Brown
0 siblings, 1 reply; 23+ messages in thread
From: Ric Wheeler @ 2007-10-31 11:06 UTC (permalink / raw)
To: Zach Brown
Cc: Mike Waychison, Chris Mason, Anton Altaparmakov, linux-fsdevel,
linux-kernel
Zach Brown wrote:
>> Can you clarify what you mean above with an example? I don't really
>> follow.
>
> Sure, take 'tar' as an example. It'll read files in the order that
> their names are returned from directory listing. This can produce bad
> IO patterns because the order in which the file names are returned
> doesn't match the order of the file's blocks on disk. (htree, I'm
> looking at you!)
>
> People have noticed that tar-like loads can be sped up greatly just by
> sorting the files by their inode number as returned by stat(), never
> mind the file blocks themselves. One example of this is Chris Mason's
> 'acp'.
>
> http://oss.oracle.com/~mason/acp/
>
> The logical extension of that is to use FIBMAP to find the order of file
> blocks on disk and then doing IO on blocks in sorted order. It'd take
> work to write an app that does this reliably, sure.
>
> In this use the application doesn't actually care what the absolute
> numbers are. It cares about their ordering. File systems would be able
> to chose whatever scheme they wanted for the actual values of the
> results from a FIBMAP-alike as long as the sorting resulted in the right
> IO patterns.
>
> Arguing that this use is significant enough to justify an addition to
> the file system API is a stretch. I'm just sharing the observation.
>
> - z
I use FIBMAP support for a few different things.
The first is to exactly the case that you describe above where we can
use the first block of a file extracted by FIBMAP to produce an optimal
sorting for the read order. My testing showed that the cost of the
extra fibmap was not too high compared to the speedup, but it was not a
huge gain over the speedup gained when the read was done in inode sorted
order.
The second use case is to look at the physical layout of blocks on disk
for a specific file, use Mark Lord's write_long patches to inject a disk
error and then read that file to make sure that we are handling disk IO
errors correctly. A bit obscure, but really quite useful.
We have also used FIBMAP a few times to try and map an observed IO error
back to a file. Really slow and painful to do, but should work on any
file system when a better method is not supported.
ric
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch 0/6][RFC] Cleanup FIBMAP
2007-10-31 11:06 ` Ric Wheeler
@ 2007-10-31 16:16 ` Zach Brown
2007-10-31 17:17 ` Ric Wheeler
0 siblings, 1 reply; 23+ messages in thread
From: Zach Brown @ 2007-10-31 16:16 UTC (permalink / raw)
To: Ric Wheeler
Cc: Mike Waychison, Chris Mason, Anton Altaparmakov, linux-fsdevel,
linux-kernel
> The second use case is to look at the physical layout of blocks on disk
> for a specific file, use Mark Lord's write_long patches to inject a disk
> error and then read that file to make sure that we are handling disk IO
> errors correctly. A bit obscure, but really quite useful.
Hmm, yeah, that's interesting.
> We have also used FIBMAP a few times to try and map an observed IO error
> back to a file. Really slow and painful to do, but should work on any
> file system when a better method is not supported.
We're getting off of this FIBMAP topic, but this interests me. Can we
explore this a little? How did you find out about the error without
having a file to associate with it? Drive scrubbing, or some such?
- z
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch 0/6][RFC] Cleanup FIBMAP
2007-10-31 16:16 ` Zach Brown
@ 2007-10-31 17:17 ` Ric Wheeler
0 siblings, 0 replies; 23+ messages in thread
From: Ric Wheeler @ 2007-10-31 17:17 UTC (permalink / raw)
To: Zach Brown
Cc: Mike Waychison, Chris Mason, Anton Altaparmakov, linux-fsdevel,
linux-kernel
Zach Brown wrote:
>> The second use case is to look at the physical layout of blocks on disk
>> for a specific file, use Mark Lord's write_long patches to inject a disk
>> error and then read that file to make sure that we are handling disk IO
>> errors correctly. A bit obscure, but really quite useful.
>
> Hmm, yeah, that's interesting.
It would be even better if we could poke holes in metadata, etc, but
this gives us a reasonable test case.
>
>> We have also used FIBMAP a few times to try and map an observed IO error
>> back to a file. Really slow and painful to do, but should work on any
>> file system when a better method is not supported.
>
> We're getting off of this FIBMAP topic, but this interests me. Can we
> explore this a little? How did you find out about the error without
> having a file to associate with it? Drive scrubbing, or some such?
>
> - z
Vladimir extended debugreiserfs to do an optimized reverse mapping scan
of the disk sector to file/metadata/etc. Definitely worth having that
ability for any file system.
We also do drive scrubbing, looking for bad sectors. The list of those
sectors is fed into the reverse mapping code to enable us to gage the
impact of the IO errors, start recovering the user files, etc.
The scrub code we use takes advantage of the read-verify command (to
avoid data transfer from the drive to the page cache).
ric
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch 0/6][RFC] Cleanup FIBMAP
2007-10-29 14:10 ` Chris Mason
2007-10-29 16:30 ` Zach Brown
@ 2007-10-29 19:16 ` Mike Waychison
2007-10-29 19:47 ` Andreas Dilger
1 sibling, 1 reply; 23+ messages in thread
From: Mike Waychison @ 2007-10-29 19:16 UTC (permalink / raw)
To: Chris Mason; +Cc: Anton Altaparmakov, linux-fsdevel, linux-kernel, adilger
Chris Mason wrote:
> On Sat, 27 Oct 2007 18:57:06 +0100
> Anton Altaparmakov <aia21@cam.ac.uk> wrote:
>
>> Hi,
>>
>> ->bmap is ugly and horrible! If you have to do this at the very
>> least please cause ->bmap64 to be able to return error values in case
>> the file system failed to get the information or indeed such
>> information does not exist as is the case for compressed and
>> encrypted files for example and also for small files that are inside
>> the on-disk inode (NTFS resident files and reiserfs packed tails are
>> examples of this).
>>
>> And another of my pet peeves with ->bmap is that it uses 0 to mean
>> "sparse" which causes a conflict on NTFS at least as block zero is
>> part of the $Boot system file so it is a real, valid block... NTFS
>> uses -1 to denote sparse blocks internally.
>
> Reiserfs and Btrfs also use 0 to mean packed. It would be nice if there
> was a way to indicate your-data-is-here-but-isn't-alone. But that's
> more of a feature for the FIEMAP stuff.
>
I hadn't heard of FIEMAP, so I went back and read the thread from
April/May. It seems that this is a much better approach than
introducing a FIBMAP64.
What ever happened with this proposal?
Mike Waychison
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch 0/6][RFC] Cleanup FIBMAP
2007-10-29 19:16 ` Mike Waychison
@ 2007-10-29 19:47 ` Andreas Dilger
0 siblings, 0 replies; 23+ messages in thread
From: Andreas Dilger @ 2007-10-29 19:47 UTC (permalink / raw)
To: Mike Waychison
Cc: Chris Mason, Anton Altaparmakov, linux-fsdevel, linux-kernel,
Kalpak Shah
On Oct 29, 2007 12:16 -0700, Mike Waychison wrote:
> Chris Mason wrote:
>> Reiserfs and Btrfs also use 0 to mean packed. It would be nice if there
>> was a way to indicate your-data-is-here-but-isn't-alone. But that's
>> more of a feature for the FIEMAP stuff.
>
> I hadn't heard of FIEMAP, so I went back and read the thread from
> April/May. It seems that this is a much better approach than introducing a
> FIBMAP64.
>
> What ever happened with this proposal?
We made a patch for ext4 that we need to update and push upstream. I've
just resent the spec we used in a separate email (attached to old thread)
for reference.
Cheers, Andreas
--
Andreas Dilger
Sr. Software Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch 0/6][RFC] Cleanup FIBMAP
2007-10-26 23:37 [patch 0/6][RFC] Cleanup FIBMAP Mike Waychison
` (6 preceding siblings ...)
2007-10-27 17:57 ` [patch 0/6][RFC] Cleanup FIBMAP Anton Altaparmakov
@ 2007-10-28 0:43 ` H. Peter Anvin
7 siblings, 0 replies; 23+ messages in thread
From: H. Peter Anvin @ 2007-10-28 0:43 UTC (permalink / raw)
To: Mike Waychison; +Cc: linux-fsdevel, linux-kernel
Mike Waychison wrote:
> The following series is meant to clean up FIBMAP paths with the eventual goal of allowing users to be able to FIBMAP their data.
Keep in mind FIBMAP is currently extremely expensive on some
filesystems, e.g. ext3. Therefore, additional filesystem-level work
would have to be done in order for this not become a DoS issue.
-hpa
^ permalink raw reply [flat|nested] 23+ messages in thread