Re: [PATCH 8/8] xfsprogs: xfs_db: add new "resvsp" command

[Dave Chinner <david@fromorbit.com>]

On Thu, Nov 10, 2011 at 02:35:18PM -0600, Alex Elder wrote:
> From: Kevan Rehm <kfr@sgi.com>
> 
> This patch adds a new "resvsp" command to xfs_db.  The command
> provides access to the xfsctl(3) XFS_IOC_RESVSP64 operation, which
> allocates space in an ordinary file.  Blocks are allocated but not
> zeroed, and the file size does not change.

The commit message fails to answer the most important question of
all: what is the use case that requires adding offline preallocation
of extents to arbitrary inodes?

Conceptually, I don't think high level functionality like extent
allocation belongs in xfs_db:

	- extent allocation is effectively an open-ended, multi-step
	  modification that can touch large amounts of metadata
	  across all AGs in the filesystem,

	- userspace extent allocation code is rarely used and as
	  such nowhere near as well tested as the kernel code.

	- userspace extent allocation does not support real time
	  device allocation at all:

	  #define xfs_bmap_rtalloc(a)                     (ENOSYS)

	- modifications made by xfs_db are not transactional and
	  therefore not recoverable if the system dies during such
	  an operation. Such a failure means you have to run a
	  xfs_reapir pass on your filesystem to clean up the mess
	  that was left behind....

	- we already have tools to do space reservation online
	  (i.e. xfs_io -c "resvsp off len" testfile) in a safe
	  manner that also supports realtime device allocation. Why
	  duplicate this functionality in a different tool?


.....

> +static void
> +resvsp_help(void)
> +{
> +	dbprintf(_(
> +"\n"
> +"The resvsp function is essentially an implementation of the xfsctl(3)\n"
> +"file operation XFS_IOC_RESVSP64 which allocates space in an ordinary\n"
> +"file.

Probably no need to mention the ioctl here - preallocation is a well
enough known function now that simply calling it "an extent
preallocation operation" is sufficient to explain what it is for.

> Blocks are allocated but not zeroed, and the file size does not\n"
> +"change.  The -o option is the starting offset for the allocation (default 0)\n"
> +"and the -l option gives the length of the allocation in bytes (default to\n"
> +"end of file).  Both offset and length values will be rounded to a filesystem\n"
> +"block boundary.  'inode#' is the inode number of the file in which to\n"
> +"perform the allocation.  If none is specified, the current inode is used.\n"
> +"If the -w option is specified, the allocated extents will not be flagged as\n"
> +"unwritten.  Use this option with care, as someone with read permission\n"
> +"to the file can then read whatever is written in those blocks.\n"

I'm on record as being totally against making it easy for -anyone-
to preallocate _written_ extents via -any method- because it's just
a whopping great big security hole. "Use care" is not a good enough
protection against the potential of exposing large amount of stale
data to any user that can read the file, even though only root can
do that preallocation.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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