2014-09-09 14:49:53

by Anna Schumaker

[permalink] [raw]
Subject: [PATCH 0/3] NFSD: Add ALLOCATE and DEALLOCATE support

From: Anna Schumaker <[email protected]>

These patches add server support for the ALLOCATE and DEALLOCATE operations
part of NFS v4.2.

The first patch in this series renames a VFS-level function for consistency
with how other VFS functions are named. Who is the correct person to carry
this patch?

The remaining patches add the new operations.

Anna

Anna Schumaker (3):
VFS: Rename do_fallocate() to vfs_fallocate()
nfsd: Add ALLOCATE support
nfsd: Add DEALLOCATE support

drivers/staging/android/ashmem.c | 2 +-
fs/ioctl.c | 2 +-
fs/nfsd/nfs4proc.c | 58 ++++++++++++++++++++++++++++++++++++++++
fs/nfsd/nfs4xdr.c | 38 ++++++++++++++++++++++++--
fs/nfsd/vfs.c | 13 +++++++++
fs/nfsd/vfs.h | 1 +
fs/nfsd/xdr4.h | 16 +++++++++++
fs/open.c | 5 ++--
include/linux/fs.h | 2 +-
mm/madvise.c | 2 +-
10 files changed, 131 insertions(+), 8 deletions(-)

--
2.1.0



2014-09-09 14:49:50

by Anna Schumaker

[permalink] [raw]
Subject: [PATCH 3/3] nfsd: Add DEALLOCATE support

From: Anna Schumaker <[email protected]>

The DEALLOCATE operation is used to punch holes in a file.

Signed-off-by: Anna Schumaker <[email protected]>
---
fs/nfsd/nfs4proc.c | 31 ++++++++++++++++++++++++++++++-
fs/nfsd/nfs4xdr.c | 19 ++++++++++++++++++-
fs/nfsd/vfs.c | 8 ++++++--
fs/nfsd/vfs.h | 2 +-
fs/nfsd/xdr4.h | 8 ++++++++
5 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index f9d8fac..ac18be6 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1028,7 +1028,7 @@ nfsd4_allocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
return status;
}

- status = nfsd4_vfs_fallocate(&cstate->current_fh, file,
+ status = nfsd4_vfs_fallocate(&cstate->current_fh, file, false,
allocate->alloc_offset,
allocate->alloc_length);
fput(file);
@@ -1036,6 +1036,28 @@ nfsd4_allocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
}

static __be32
+nfsd4_deallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
+ struct nfsd4_deallocate *deallocate)
+{
+ __be32 status = nfserr_notsupp;
+ struct file *file;
+
+ status = nfs4_preprocess_stateid_op(SVC_NET(rqstp), cstate,
+ &deallocate->dealloc_stateid,
+ WR_STATE, &file);
+ if (status != nfs_ok) {
+ dprintk("NFSD: nfsd4_deallocate: couldn't process stateid!\n");
+ return status;
+ }
+
+ status = nfsd4_vfs_fallocate(&cstate->current_fh, file, true,
+ deallocate->dealloc_offset,
+ deallocate->dealloc_length);
+ fput(file);
+ return status;
+}
+
+static __be32
nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
struct nfsd4_seek *seek)
{
@@ -1949,6 +1971,13 @@ static struct nfsd4_operation nfsd4_ops[] = {
.op_rsize_bop = (nfsd4op_rsize)nfsd4_write_rsize,
.op_get_currentstateid = (stateid_getter)nfsd4_get_writestateid,
},
+ [OP_DEALLOCATE] = {
+ .op_func = (nfsd4op_func)nfsd4_deallocate,
+ .op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME,
+ .op_name = "OP_DEALLOCATE",
+ .op_rsize_bop = (nfsd4op_rsize)nfsd4_write_rsize,
+ .op_get_currentstateid = (stateid_getter)nfsd4_get_writestateid,
+ },
[OP_SEEK] = {
.op_func = (nfsd4op_func)nfsd4_seek,
.op_name = "OP_SEEK",
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index a55aa3b..e1059ce 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1538,6 +1538,23 @@ nfsd4_decode_allocate(struct nfsd4_compoundargs *argp,
}

static __be32
+nfsd4_decode_deallocate(struct nfsd4_compoundargs *argp,
+ struct nfsd4_deallocate *deallocate)
+{
+ DECODE_HEAD;
+
+ status = nfsd4_decode_stateid(argp, &deallocate->dealloc_stateid);
+ if (status)
+ return status;
+
+ READ_BUF(16);
+ p = xdr_decode_hyper(p, &deallocate->dealloc_offset);
+ xdr_decode_hyper(p, &deallocate->dealloc_length);
+
+ DECODE_TAIL;
+}
+
+static __be32
nfsd4_decode_seek(struct nfsd4_compoundargs *argp, struct nfsd4_seek *seek)
{
DECODE_HEAD;
@@ -1631,7 +1648,7 @@ static nfsd4_dec nfsd4_dec_ops[] = {
[OP_ALLOCATE] = (nfsd4_dec)nfsd4_decode_allocate,
[OP_COPY] = (nfsd4_dec)nfsd4_decode_notsupp,
[OP_COPY_NOTIFY] = (nfsd4_dec)nfsd4_decode_notsupp,
- [OP_DEALLOCATE] = (nfsd4_dec)nfsd4_decode_notsupp,
+ [OP_DEALLOCATE] = (nfsd4_dec)nfsd4_decode_deallocate,
[OP_IO_ADVISE] = (nfsd4_dec)nfsd4_decode_notsupp,
[OP_LAYOUTERROR] = (nfsd4_dec)nfsd4_decode_notsupp,
[OP_LAYOUTSTATS] = (nfsd4_dec)nfsd4_decode_notsupp,
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 4b1dba0..2bf5263 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -525,9 +525,13 @@ __be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
#endif

__be32 nfsd4_vfs_fallocate(struct svc_fh *fhp, struct file *file,
- loff_t offset, loff_t len)
+ bool punch, loff_t offset, loff_t len)
{
- int error = vfs_fallocate(file, 0, offset, len);
+ int error, mode = 0;
+ if (punch)
+ mode = FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE;
+
+ error = vfs_fallocate(file, mode, offset, len);
if (error < 0)
return nfserrno(error);
return nfserrno(commit_metadata(fhp));
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 9d88c88..9301e8e 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -54,7 +54,7 @@ int nfsd_mountpoint(struct dentry *, struct svc_export *);
#ifdef CONFIG_NFSD_V4
__be32 nfsd4_set_nfs4_label(struct svc_rqst *, struct svc_fh *,
struct xdr_netobj *);
-__be32 nfsd4_vfs_fallocate(struct svc_fh *, struct file *, loff_t, loff_t);
+__be32 nfsd4_vfs_fallocate(struct svc_fh *, struct file *, bool, loff_t, loff_t);
#endif /* CONFIG_NFSD_V4 */
__be32 nfsd_create(struct svc_rqst *, struct svc_fh *,
char *name, int len, struct iattr *attrs,
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 4a01fa8..2866921 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -435,6 +435,13 @@ struct nfsd4_allocate {
u64 alloc_length;
};

+struct nfsd4_deallocate {
+ /* request */
+ stateid_t dealloc_stateid;
+ loff_t dealloc_offset;
+ u64 dealloc_length;
+};
+
struct nfsd4_seek {
/* request */
stateid_t seek_stateid;
@@ -494,6 +501,7 @@ struct nfsd4_op {

/* NFSv4.2 */
struct nfsd4_allocate allocate;
+ struct nfsd4_deallocate deallocate;
struct nfsd4_seek seek;
} u;
struct nfs4_replay * replay;
--
2.1.0


2014-09-09 19:00:40

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 0/3] NFSD: Add ALLOCATE and DEALLOCATE support

On Tue, Sep 09, 2014 at 11:13:02AM -0700, Christoph Hellwig wrote:
> Sorry to hijack this thread, but talking about 4.2 server features, did
> anyone into looking into implementing the change_attr_type attribute,
> which should be fairly low hanging fruit?

I haven't seen anything. Agreed that it should be easy (on the server
side--the client's use might be more complicated?).

--b.

>
> Also someone with interest in btrfs might want to look into implementing
> the space_freed attribute with a call into the filesystem.
>

2014-09-09 18:13:02

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 0/3] NFSD: Add ALLOCATE and DEALLOCATE support

Sorry to hijack this thread, but talking about 4.2 server features, did
anyone into looking into implementing the change_attr_type attribute,
which should be fairly low hanging fruit?

Also someone with interest in btrfs might want to look into implementing
the space_freed attribute with a call into the filesystem.


2014-09-10 22:12:02

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 1/3] VFS: Rename do_fallocate() to vfs_fallocate()

On Wed, Sep 10, 2014 at 03:43:42PM -0400, J. Bruce Fields wrote:
> On Tue, Sep 09, 2014 at 08:50:19AM -0700, Christoph Hellwig wrote:
> > On Tue, Sep 09, 2014 at 10:49:40AM -0400, [email protected] wrote:
> > > From: Anna Schumaker <[email protected]>
> > >
> > > This function needs to be exported so it can be used by the NFSD module
> > > when responding to ALLOCATE and DEALLOCATE operations. Christoph Hellwig
> > > suggested renaming the function to match how other vfs functions are named.
> > >
> > > Signed-off-by: Anna Schumaker <[email protected]>
> >
> > Looks good,
> >
> > Reviewed-by: Christoph Hellwig <[email protected]>
>
> I'd like to be able to take this into through the nfsd tree if the nfsd
> patches are going to depend on it.
>
> Anna, on next posting maybe you could make sure to cc: the reelevant
> people (I'm not sure who...) so I can get the right ACKs for that?

Cc Al and fsdevel, please.


2014-09-10 22:11:51

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 2/3] nfsd: Add ALLOCATE support

On Wed, Sep 10, 2014 at 04:18:12PM -0400, J. Bruce Fields wrote:
> This checks that the stateid's openmode is write but not that the caller
> has permission to write. I think you want an nfsd_permission() check in
> nfsd4_vfs_allocate.
>
> (Incidentally, the spec doesn't say anything about checking either, I
> wonder if it should?)

Yes, it probably should.


2014-09-09 14:49:50

by Anna Schumaker

[permalink] [raw]
Subject: [PATCH 2/3] nfsd: Add ALLOCATE support

From: Anna Schumaker <[email protected]>

The ALLOCATE operation is used to preallocate space in a file. I use
vfs_fallocate() to do the actual preallocation.

Signed-off-by: Anna Schumaker <[email protected]>
---
fs/nfsd/nfs4proc.c | 29 +++++++++++++++++++++++++++++
fs/nfsd/nfs4xdr.c | 19 ++++++++++++++++++-
fs/nfsd/vfs.c | 9 +++++++++
fs/nfsd/vfs.h | 1 +
fs/nfsd/xdr4.h | 8 ++++++++
5 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 9fd2b78..f9d8fac 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1014,6 +1014,28 @@ nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
}

static __be32
+nfsd4_allocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
+ struct nfsd4_allocate *allocate)
+{
+ __be32 status = nfserr_notsupp;
+ struct file *file;
+
+ status = nfs4_preprocess_stateid_op(SVC_NET(rqstp), cstate,
+ &allocate->alloc_stateid,
+ WR_STATE, &file);
+ if (status != nfs_ok) {
+ dprintk("NFSD: nfsd4_allocate: couldn't process stateid!\n");
+ return status;
+ }
+
+ status = nfsd4_vfs_fallocate(&cstate->current_fh, file,
+ allocate->alloc_offset,
+ allocate->alloc_length);
+ fput(file);
+ return status;
+}
+
+static __be32
nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
struct nfsd4_seek *seek)
{
@@ -1920,6 +1942,13 @@ static struct nfsd4_operation nfsd4_ops[] = {
},

/* NFSv4.2 operations */
+ [OP_ALLOCATE] = {
+ .op_func = (nfsd4op_func)nfsd4_allocate,
+ .op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME,
+ .op_name = "OP_ALLOCATE",
+ .op_rsize_bop = (nfsd4op_rsize)nfsd4_write_rsize,
+ .op_get_currentstateid = (stateid_getter)nfsd4_get_writestateid,
+ },
[OP_SEEK] = {
.op_func = (nfsd4op_func)nfsd4_seek,
.op_name = "OP_SEEK",
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 57b3973..a55aa3b 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1521,6 +1521,23 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
}

static __be32
+nfsd4_decode_allocate(struct nfsd4_compoundargs *argp,
+ struct nfsd4_allocate *allocate)
+{
+ DECODE_HEAD;
+
+ status = nfsd4_decode_stateid(argp, &allocate->alloc_stateid);
+ if (status)
+ return status;
+
+ READ_BUF(16);
+ p = xdr_decode_hyper(p, &allocate->alloc_offset);
+ xdr_decode_hyper(p, &allocate->alloc_length);
+
+ DECODE_TAIL;
+}
+
+static __be32
nfsd4_decode_seek(struct nfsd4_compoundargs *argp, struct nfsd4_seek *seek)
{
DECODE_HEAD;
@@ -1611,7 +1628,7 @@ static nfsd4_dec nfsd4_dec_ops[] = {
[OP_RECLAIM_COMPLETE] = (nfsd4_dec)nfsd4_decode_reclaim_complete,

/* new operations for NFSv4.2 */
- [OP_ALLOCATE] = (nfsd4_dec)nfsd4_decode_notsupp,
+ [OP_ALLOCATE] = (nfsd4_dec)nfsd4_decode_allocate,
[OP_COPY] = (nfsd4_dec)nfsd4_decode_notsupp,
[OP_COPY_NOTIFY] = (nfsd4_dec)nfsd4_decode_notsupp,
[OP_DEALLOCATE] = (nfsd4_dec)nfsd4_decode_notsupp,
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index f501a9b..4b1dba0 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -16,6 +16,7 @@
#include <linux/fs.h>
#include <linux/file.h>
#include <linux/splice.h>
+#include <linux/falloc.h>
#include <linux/fcntl.h>
#include <linux/namei.h>
#include <linux/delay.h>
@@ -523,6 +524,14 @@ __be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
}
#endif

+__be32 nfsd4_vfs_fallocate(struct svc_fh *fhp, struct file *file,
+ loff_t offset, loff_t len)
+{
+ int error = vfs_fallocate(file, 0, offset, len);
+ if (error < 0)
+ return nfserrno(error);
+ return nfserrno(commit_metadata(fhp));
+}
#endif /* defined(CONFIG_NFSD_V4) */

#ifdef CONFIG_NFSD_V3
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index c2ff3f1..9d88c88 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -54,6 +54,7 @@ int nfsd_mountpoint(struct dentry *, struct svc_export *);
#ifdef CONFIG_NFSD_V4
__be32 nfsd4_set_nfs4_label(struct svc_rqst *, struct svc_fh *,
struct xdr_netobj *);
+__be32 nfsd4_vfs_fallocate(struct svc_fh *, struct file *, loff_t, loff_t);
#endif /* CONFIG_NFSD_V4 */
__be32 nfsd_create(struct svc_rqst *, struct svc_fh *,
char *name, int len, struct iattr *attrs,
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 5720e94..4a01fa8 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -428,6 +428,13 @@ struct nfsd4_reclaim_complete {
u32 rca_one_fs;
};

+struct nfsd4_allocate {
+ /* request */
+ stateid_t alloc_stateid;
+ loff_t alloc_offset;
+ u64 alloc_length;
+};
+
struct nfsd4_seek {
/* request */
stateid_t seek_stateid;
@@ -486,6 +493,7 @@ struct nfsd4_op {
struct nfsd4_free_stateid free_stateid;

/* NFSv4.2 */
+ struct nfsd4_allocate allocate;
struct nfsd4_seek seek;
} u;
struct nfs4_replay * replay;
--
2.1.0


2014-09-09 15:56:27

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 3/3] nfsd: Add DEALLOCATE support

> - status = nfsd4_vfs_fallocate(&cstate->current_fh, file,
> + status = nfsd4_vfs_fallocate(&cstate->current_fh, file, false,
> allocate->alloc_offset,
> allocate->alloc_length);

Maye just pass the normal Linux fallocate flags to nfsd4_vfs_fallocate
for future extensibility?

Otherwise looks as good as the previous one.


2014-09-09 15:50:22

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 1/3] VFS: Rename do_fallocate() to vfs_fallocate()

On Tue, Sep 09, 2014 at 10:49:40AM -0400, [email protected] wrote:
> From: Anna Schumaker <[email protected]>
>
> This function needs to be exported so it can be used by the NFSD module
> when responding to ALLOCATE and DEALLOCATE operations. Christoph Hellwig
> suggested renaming the function to match how other vfs functions are named.
>
> Signed-off-by: Anna Schumaker <[email protected]>

Looks good,

Reviewed-by: Christoph Hellwig <[email protected]>

2014-09-09 14:49:49

by Anna Schumaker

[permalink] [raw]
Subject: [PATCH 1/3] VFS: Rename do_fallocate() to vfs_fallocate()

From: Anna Schumaker <[email protected]>

This function needs to be exported so it can be used by the NFSD module
when responding to ALLOCATE and DEALLOCATE operations. Christoph Hellwig
suggested renaming the function to match how other vfs functions are named.

Signed-off-by: Anna Schumaker <[email protected]>
---
drivers/staging/android/ashmem.c | 2 +-
fs/ioctl.c | 2 +-
fs/open.c | 5 +++--
include/linux/fs.h | 2 +-
mm/madvise.c | 2 +-
5 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 713a972..9c799987 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -446,7 +446,7 @@ ashmem_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
loff_t start = range->pgstart * PAGE_SIZE;
loff_t end = (range->pgend + 1) * PAGE_SIZE;

- do_fallocate(range->asma->file,
+ vfs_fallocate(range->asma->file,
FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
start, end - start);
range->purged = ASHMEM_WAS_PURGED;
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 8ac3fad..0bd61421 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -443,7 +443,7 @@ int ioctl_preallocate(struct file *filp, void __user *argp)
return -EINVAL;
}

- return do_fallocate(filp, FALLOC_FL_KEEP_SIZE, sr.l_start, sr.l_len);
+ return vfs_fallocate(filp, FALLOC_FL_KEEP_SIZE, sr.l_start, sr.l_len);
}

static int file_ioctl(struct file *filp, unsigned int cmd,
diff --git a/fs/open.c b/fs/open.c
index d6fd3ac..c94449b 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -222,7 +222,7 @@ SYSCALL_DEFINE2(ftruncate64, unsigned int, fd, loff_t, length)
#endif /* BITS_PER_LONG == 32 */


-int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
+int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
{
struct inode *inode = file_inode(file);
long ret;
@@ -298,6 +298,7 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
sb_end_write(inode->i_sb);
return ret;
}
+EXPORT_SYMBOL_GPL(vfs_fallocate);

SYSCALL_DEFINE4(fallocate, int, fd, int, mode, loff_t, offset, loff_t, len)
{
@@ -305,7 +306,7 @@ SYSCALL_DEFINE4(fallocate, int, fd, int, mode, loff_t, offset, loff_t, len)
int error = -EBADF;

if (f.file) {
- error = do_fallocate(f.file, mode, offset, len);
+ error = vfs_fallocate(f.file, mode, offset, len);
fdput(f);
}
return error;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9418772..476f555 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2049,7 +2049,7 @@ struct filename {
extern long vfs_truncate(struct path *, loff_t);
extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs,
struct file *filp);
-extern int do_fallocate(struct file *file, int mode, loff_t offset,
+extern int vfs_fallocate(struct file *file, int mode, loff_t offset,
loff_t len);
extern long do_sys_open(int dfd, const char __user *filename, int flags,
umode_t mode);
diff --git a/mm/madvise.c b/mm/madvise.c
index 0938b30..a271adc 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -326,7 +326,7 @@ static long madvise_remove(struct vm_area_struct *vma,
*/
get_file(f);
up_read(&current->mm->mmap_sem);
- error = do_fallocate(f,
+ error = vfs_fallocate(f,
FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
offset, end - start);
fput(f);
--
2.1.0


2014-09-10 22:13:04

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 3/3] nfsd: Add DEALLOCATE support

On Wed, Sep 10, 2014 at 04:19:05PM -0400, J. Bruce Fields wrote:
> Same comment as in allocate case.

The nfsd_permission check probably should go into nfsd_vfs_fallocate
to avoid duplication.

2014-09-10 20:19:06

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 3/3] nfsd: Add DEALLOCATE support

On Tue, Sep 09, 2014 at 10:49:42AM -0400, [email protected] wrote:
> From: Anna Schumaker <[email protected]>
>
> The DEALLOCATE operation is used to punch holes in a file.
>
> Signed-off-by: Anna Schumaker <[email protected]>
> ---
> fs/nfsd/nfs4proc.c | 31 ++++++++++++++++++++++++++++++-
> fs/nfsd/nfs4xdr.c | 19 ++++++++++++++++++-
> fs/nfsd/vfs.c | 8 ++++++--
> fs/nfsd/vfs.h | 2 +-
> fs/nfsd/xdr4.h | 8 ++++++++
> 5 files changed, 63 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index f9d8fac..ac18be6 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1028,7 +1028,7 @@ nfsd4_allocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> return status;
> }
>
> - status = nfsd4_vfs_fallocate(&cstate->current_fh, file,
> + status = nfsd4_vfs_fallocate(&cstate->current_fh, file, false,
> allocate->alloc_offset,
> allocate->alloc_length);
> fput(file);
> @@ -1036,6 +1036,28 @@ nfsd4_allocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> }
>
> static __be32
> +nfsd4_deallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> + struct nfsd4_deallocate *deallocate)
> +{
> + __be32 status = nfserr_notsupp;
> + struct file *file;
> +
> + status = nfs4_preprocess_stateid_op(SVC_NET(rqstp), cstate,
> + &deallocate->dealloc_stateid,
> + WR_STATE, &file);
> + if (status != nfs_ok) {
> + dprintk("NFSD: nfsd4_deallocate: couldn't process stateid!\n");
> + return status;
> + }
> +
> + status = nfsd4_vfs_fallocate(&cstate->current_fh, file, true,
> + deallocate->dealloc_offset,
> + deallocate->dealloc_length);

Same comment as in allocate case.

--b.

> + fput(file);
> + return status;
> +}
> +
> +static __be32
> nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> struct nfsd4_seek *seek)
> {
> @@ -1949,6 +1971,13 @@ static struct nfsd4_operation nfsd4_ops[] = {
> .op_rsize_bop = (nfsd4op_rsize)nfsd4_write_rsize,
> .op_get_currentstateid = (stateid_getter)nfsd4_get_writestateid,
> },
> + [OP_DEALLOCATE] = {
> + .op_func = (nfsd4op_func)nfsd4_deallocate,
> + .op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME,
> + .op_name = "OP_DEALLOCATE",
> + .op_rsize_bop = (nfsd4op_rsize)nfsd4_write_rsize,
> + .op_get_currentstateid = (stateid_getter)nfsd4_get_writestateid,
> + },
> [OP_SEEK] = {
> .op_func = (nfsd4op_func)nfsd4_seek,
> .op_name = "OP_SEEK",
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index a55aa3b..e1059ce 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1538,6 +1538,23 @@ nfsd4_decode_allocate(struct nfsd4_compoundargs *argp,
> }
>
> static __be32
> +nfsd4_decode_deallocate(struct nfsd4_compoundargs *argp,
> + struct nfsd4_deallocate *deallocate)
> +{
> + DECODE_HEAD;
> +
> + status = nfsd4_decode_stateid(argp, &deallocate->dealloc_stateid);
> + if (status)
> + return status;
> +
> + READ_BUF(16);
> + p = xdr_decode_hyper(p, &deallocate->dealloc_offset);
> + xdr_decode_hyper(p, &deallocate->dealloc_length);
> +
> + DECODE_TAIL;
> +}
> +
> +static __be32
> nfsd4_decode_seek(struct nfsd4_compoundargs *argp, struct nfsd4_seek *seek)
> {
> DECODE_HEAD;
> @@ -1631,7 +1648,7 @@ static nfsd4_dec nfsd4_dec_ops[] = {
> [OP_ALLOCATE] = (nfsd4_dec)nfsd4_decode_allocate,
> [OP_COPY] = (nfsd4_dec)nfsd4_decode_notsupp,
> [OP_COPY_NOTIFY] = (nfsd4_dec)nfsd4_decode_notsupp,
> - [OP_DEALLOCATE] = (nfsd4_dec)nfsd4_decode_notsupp,
> + [OP_DEALLOCATE] = (nfsd4_dec)nfsd4_decode_deallocate,
> [OP_IO_ADVISE] = (nfsd4_dec)nfsd4_decode_notsupp,
> [OP_LAYOUTERROR] = (nfsd4_dec)nfsd4_decode_notsupp,
> [OP_LAYOUTSTATS] = (nfsd4_dec)nfsd4_decode_notsupp,
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 4b1dba0..2bf5263 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -525,9 +525,13 @@ __be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
> #endif
>
> __be32 nfsd4_vfs_fallocate(struct svc_fh *fhp, struct file *file,
> - loff_t offset, loff_t len)
> + bool punch, loff_t offset, loff_t len)
> {
> - int error = vfs_fallocate(file, 0, offset, len);
> + int error, mode = 0;
> + if (punch)
> + mode = FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE;
> +
> + error = vfs_fallocate(file, mode, offset, len);
> if (error < 0)
> return nfserrno(error);
> return nfserrno(commit_metadata(fhp));
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 9d88c88..9301e8e 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -54,7 +54,7 @@ int nfsd_mountpoint(struct dentry *, struct svc_export *);
> #ifdef CONFIG_NFSD_V4
> __be32 nfsd4_set_nfs4_label(struct svc_rqst *, struct svc_fh *,
> struct xdr_netobj *);
> -__be32 nfsd4_vfs_fallocate(struct svc_fh *, struct file *, loff_t, loff_t);
> +__be32 nfsd4_vfs_fallocate(struct svc_fh *, struct file *, bool, loff_t, loff_t);
> #endif /* CONFIG_NFSD_V4 */
> __be32 nfsd_create(struct svc_rqst *, struct svc_fh *,
> char *name, int len, struct iattr *attrs,
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 4a01fa8..2866921 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -435,6 +435,13 @@ struct nfsd4_allocate {
> u64 alloc_length;
> };
>
> +struct nfsd4_deallocate {
> + /* request */
> + stateid_t dealloc_stateid;
> + loff_t dealloc_offset;
> + u64 dealloc_length;
> +};
> +
> struct nfsd4_seek {
> /* request */
> stateid_t seek_stateid;
> @@ -494,6 +501,7 @@ struct nfsd4_op {
>
> /* NFSv4.2 */
> struct nfsd4_allocate allocate;
> + struct nfsd4_deallocate deallocate;
> struct nfsd4_seek seek;
> } u;
> struct nfs4_replay * replay;
> --
> 2.1.0
>

2014-09-09 15:55:19

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 2/3] nfsd: Add ALLOCATE support

On Tue, Sep 09, 2014 at 10:49:41AM -0400, [email protected] wrote:
> From: Anna Schumaker <[email protected]>
>
> The ALLOCATE operation is used to preallocate space in a file. I use
> vfs_fallocate() to do the actual preallocation.
>
> Signed-off-by: Anna Schumaker <[email protected]>

Looks largely good, but can you do an audit how the error codes match
the spec? E.g. We return -ENODEV in do_fallocate if it's called on
a special file, but that doesn't translate to any valid error codes
for ALLOCATE in the NFSv4.2.

I guess it should be NFSERR_NOTSUPP, and I'll work with Tom to actually
define this case in the spec.

2014-09-09 19:17:09

by Anna Schumaker

[permalink] [raw]
Subject: Re: [PATCH 2/3] nfsd: Add ALLOCATE support

On 09/09/2014 11:55 AM, Christoph Hellwig wrote:
> On Tue, Sep 09, 2014 at 10:49:41AM -0400, [email protected] wrote:
>> From: Anna Schumaker <[email protected]>
>>
>> The ALLOCATE operation is used to preallocate space in a file. I use
>> vfs_fallocate() to do the actual preallocation.
>>
>> Signed-off-by: Anna Schumaker <[email protected]>
>
> Looks largely good, but can you do an audit how the error codes match
> the spec? E.g. We return -ENODEV in do_fallocate if it's called on
> a special file, but that doesn't translate to any valid error codes
> for ALLOCATE in the NFSv4.2.

Good idea, I'll look into it!

Anna
>
> I guess it should be NFSERR_NOTSUPP, and I'll work with Tom to actually
> define this case in the spec.
>


2014-09-10 22:26:10

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 2/3] nfsd: Add ALLOCATE support

On Wed, Sep 10, 2014 at 03:11:50PM -0700, Christoph Hellwig wrote:
> On Wed, Sep 10, 2014 at 04:18:12PM -0400, J. Bruce Fields wrote:
> > This checks that the stateid's openmode is write but not that the caller
> > has permission to write. I think you want an nfsd_permission() check in
> > nfsd4_vfs_allocate.
> >
> > (Incidentally, the spec doesn't say anything about checking either, I
> > wonder if it should?)
>
> Yes, it probably should.

Turns out the spec (at least 4.1) doesn't mention anything about
requiring a writeable stateid or permissions for WRITE either as
far as I can tell..

2014-09-10 20:18:14

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 2/3] nfsd: Add ALLOCATE support

On Tue, Sep 09, 2014 at 10:49:41AM -0400, [email protected] wrote:
> From: Anna Schumaker <[email protected]>
>
> The ALLOCATE operation is used to preallocate space in a file. I use
> vfs_fallocate() to do the actual preallocation.
>
> Signed-off-by: Anna Schumaker <[email protected]>
> ---
> fs/nfsd/nfs4proc.c | 29 +++++++++++++++++++++++++++++
> fs/nfsd/nfs4xdr.c | 19 ++++++++++++++++++-
> fs/nfsd/vfs.c | 9 +++++++++
> fs/nfsd/vfs.h | 1 +
> fs/nfsd/xdr4.h | 8 ++++++++
> 5 files changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 9fd2b78..f9d8fac 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1014,6 +1014,28 @@ nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> }
>
> static __be32
> +nfsd4_allocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> + struct nfsd4_allocate *allocate)
> +{
> + __be32 status = nfserr_notsupp;
> + struct file *file;
> +
> + status = nfs4_preprocess_stateid_op(SVC_NET(rqstp), cstate,
> + &allocate->alloc_stateid,
> + WR_STATE, &file);

This checks that the stateid's openmode is write but not that the caller
has permission to write. I think you want an nfsd_permission() check in
nfsd4_vfs_allocate.

(Incidentally, the spec doesn't say anything about checking either, I
wonder if it should?)

> + if (status != nfs_ok) {
> + dprintk("NFSD: nfsd4_allocate: couldn't process stateid!\n");
> + return status;
> + }
> +
> + status = nfsd4_vfs_fallocate(&cstate->current_fh, file,
> + allocate->alloc_offset,
> + allocate->alloc_length);
> + fput(file);
> + return status;
> +}
> +
> +static __be32
> nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> struct nfsd4_seek *seek)
> {
> @@ -1920,6 +1942,13 @@ static struct nfsd4_operation nfsd4_ops[] = {
> },
>
> /* NFSv4.2 operations */
> + [OP_ALLOCATE] = {
> + .op_func = (nfsd4op_func)nfsd4_allocate,
> + .op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME,
> + .op_name = "OP_ALLOCATE",
> + .op_rsize_bop = (nfsd4op_rsize)nfsd4_write_rsize,
> + .op_get_currentstateid = (stateid_getter)nfsd4_get_writestateid,
> + },
> [OP_SEEK] = {
> .op_func = (nfsd4op_func)nfsd4_seek,
> .op_name = "OP_SEEK",
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 57b3973..a55aa3b 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1521,6 +1521,23 @@ static __be32 nfsd4_decode_reclaim_complete(struct nfsd4_compoundargs *argp, str
> }
>
> static __be32
> +nfsd4_decode_allocate(struct nfsd4_compoundargs *argp,
> + struct nfsd4_allocate *allocate)
> +{
> + DECODE_HEAD;
> +
> + status = nfsd4_decode_stateid(argp, &allocate->alloc_stateid);
> + if (status)
> + return status;
> +
> + READ_BUF(16);
> + p = xdr_decode_hyper(p, &allocate->alloc_offset);
> + xdr_decode_hyper(p, &allocate->alloc_length);
> +
> + DECODE_TAIL;
> +}
> +
> +static __be32
> nfsd4_decode_seek(struct nfsd4_compoundargs *argp, struct nfsd4_seek *seek)
> {
> DECODE_HEAD;
> @@ -1611,7 +1628,7 @@ static nfsd4_dec nfsd4_dec_ops[] = {
> [OP_RECLAIM_COMPLETE] = (nfsd4_dec)nfsd4_decode_reclaim_complete,
>
> /* new operations for NFSv4.2 */
> - [OP_ALLOCATE] = (nfsd4_dec)nfsd4_decode_notsupp,
> + [OP_ALLOCATE] = (nfsd4_dec)nfsd4_decode_allocate,
> [OP_COPY] = (nfsd4_dec)nfsd4_decode_notsupp,
> [OP_COPY_NOTIFY] = (nfsd4_dec)nfsd4_decode_notsupp,
> [OP_DEALLOCATE] = (nfsd4_dec)nfsd4_decode_notsupp,
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index f501a9b..4b1dba0 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -16,6 +16,7 @@
> #include <linux/fs.h>
> #include <linux/file.h>
> #include <linux/splice.h>
> +#include <linux/falloc.h>
> #include <linux/fcntl.h>
> #include <linux/namei.h>
> #include <linux/delay.h>
> @@ -523,6 +524,14 @@ __be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
> }
> #endif
>
> +__be32 nfsd4_vfs_fallocate(struct svc_fh *fhp, struct file *file,
> + loff_t offset, loff_t len)
> +{
> + int error = vfs_fallocate(file, 0, offset, len);
> + if (error < 0)
> + return nfserrno(error);
> + return nfserrno(commit_metadata(fhp));
> +}
> #endif /* defined(CONFIG_NFSD_V4) */
>
> #ifdef CONFIG_NFSD_V3
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index c2ff3f1..9d88c88 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -54,6 +54,7 @@ int nfsd_mountpoint(struct dentry *, struct svc_export *);
> #ifdef CONFIG_NFSD_V4
> __be32 nfsd4_set_nfs4_label(struct svc_rqst *, struct svc_fh *,
> struct xdr_netobj *);
> +__be32 nfsd4_vfs_fallocate(struct svc_fh *, struct file *, loff_t, loff_t);
> #endif /* CONFIG_NFSD_V4 */
> __be32 nfsd_create(struct svc_rqst *, struct svc_fh *,
> char *name, int len, struct iattr *attrs,
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 5720e94..4a01fa8 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -428,6 +428,13 @@ struct nfsd4_reclaim_complete {
> u32 rca_one_fs;
> };
>
> +struct nfsd4_allocate {
> + /* request */
> + stateid_t alloc_stateid;
> + loff_t alloc_offset;
> + u64 alloc_length;
> +};
> +
> struct nfsd4_seek {
> /* request */
> stateid_t seek_stateid;
> @@ -486,6 +493,7 @@ struct nfsd4_op {
> struct nfsd4_free_stateid free_stateid;
>
> /* NFSv4.2 */
> + struct nfsd4_allocate allocate;
> struct nfsd4_seek seek;
> } u;
> struct nfs4_replay * replay;
> --
> 2.1.0
>

2014-09-10 19:43:46

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 1/3] VFS: Rename do_fallocate() to vfs_fallocate()

On Tue, Sep 09, 2014 at 08:50:19AM -0700, Christoph Hellwig wrote:
> On Tue, Sep 09, 2014 at 10:49:40AM -0400, [email protected] wrote:
> > From: Anna Schumaker <[email protected]>
> >
> > This function needs to be exported so it can be used by the NFSD module
> > when responding to ALLOCATE and DEALLOCATE operations. Christoph Hellwig
> > suggested renaming the function to match how other vfs functions are named.
> >
> > Signed-off-by: Anna Schumaker <[email protected]>
>
> Looks good,
>
> Reviewed-by: Christoph Hellwig <[email protected]>

I'd like to be able to take this into through the nfsd tree if the nfsd
patches are going to depend on it.

Anna, on next posting maybe you could make sure to cc: the reelevant
people (I'm not sure who...) so I can get the right ACKs for that?

--b.