Andrew,

Thanks for the review comments!

On Thu, May 03, 2007 at 09:29:55PM -0700, Andrew Morton wrote:

On Thu, 26 Apr 2007 23:33:32 +0530 "Amit K. Arora" [off-list ref] wrote:

quoted

This patch implements the fallocate() system call and adds support for
i386, x86_64 and powerpc.

...

+asmlinkage long sys_fallocate(int fd, int mode, loff_t offset, loff_t len)

Please add a comment over this function which specifies its behaviour. 
Really it should be enough material from which a full manpage can be
written.

If that's all too much, this material should at least be spelled out in the
changelog.  Because there's no way in which this change can be fully
reviewed unless someone (ie: you) tells us what it is setting out to
achieve.

If we 100% implement some standard then a URL for what we claim to
implement would suffice.  Given that we're at least using different types from
posix I doubt if such a thing would be sufficient.

And given the complexity and potential variability within the filesystem
implementations of this, I'd expect that _something_ additional needs to be
said?

Ok. I will add a detailed comment here.

quoted

+{
+	struct file *file;
+	struct inode *inode;
+	long ret = -EINVAL;
+
+	if (len == 0 || offset < 0)
+		goto out;

The posix spec implies that negative `len' is permitted - presumably "allocate
ahead of `offset'".  How peculiar.

I think we should go ahead with current glibc implementation (which
Jakub poited at) of not allowing a negative 'len', since posix also
doesn't explicitly say anything about allowing negative 'len'.

quoted

+	ret = -EBADF;
+	file = fget(fd);
+	if (!file)
+		goto out;
+	if (!(file->f_mode & FMODE_WRITE))
+		goto out_fput;
+
+	inode = file->f_path.dentry->d_inode;
+
+	ret = -ESPIPE;
+	if (S_ISFIFO(inode->i_mode))
+		goto out_fput;
+
+	ret = -ENODEV;
+	if (!S_ISREG(inode->i_mode))
+		goto out_fput;

So we return ENODEV against an S_ISBLK fd, as per the posix spec.  That
seems a bit silly of them.

True. 
 

quoted

+	ret = -EFBIG;
+	if (offset + len > inode->i_sb->s_maxbytes)
+		goto out_fput;

This code does handle offset+len going negative, but only by accident, I
suspect.  It happens that s_maxbytes has unsigned type.  Perhaps a comment
here would settle the reader's mind.

Ok. I will add a check here for wrap though zero.
 

quoted

+	if (inode->i_op && inode->i_op->fallocate)
+		ret = inode->i_op->fallocate(inode, mode, offset, len);
+	else
+		ret = -ENOSYS;

If we _are_ going to support negative `len', as posix suggests, I think we
should perform the appropriate sanity conversions to `offset' and `len'
right here, rather than expecting each filesystem to do it.

If we're not going to handle negative `len' then we should check for it.

Will add a check for negative 'len' and return -EINVAL. This will be
done where currently we check for negative offset (i.e. at the start of
the function).
 

quoted

+out_fput:
+	fput(file);
+out:
+	return ret;
+}
+EXPORT_SYMBOL(sys_fallocate);

I don't believe this needs to be exported to modules?

Ok. Will remove it.
 

quoted

+/*
+ * fallocate() modes
+ */
+#define FA_ALLOCATE	0x1
+#define FA_DEALLOCATE	0x2

Now those aren't in posix.  They should be documented, along with their
expected semantics.

Will add a comment describing the role of these modes.
 

quoted

 #ifdef __KERNEL__
 
 #include <linux/linkage.h>

@@ -1125,6 +1131,7 @@ struct inode_operations {
 	ssize_t (*listxattr) (struct dentry *, char *, size_t);
 	int (*removexattr) (struct dentry *, const char *);
 	void (*truncate_range)(struct inode *, loff_t, loff_t);
+	long (*fallocate)(struct inode *, int, loff_t, loff_t);

I really do think it's better to put the variable names in definitions such
as this.  Especially when we have two identically-typed variables next to
each other like that.  Quick: which one is the offset and which is the
length?

Ok. Will add the variable names here.

--
Regards,
Amit Arora