Trond Myklebust wrote:
> On Mon, 2009-04-20 at 11:09 +0530, Suresh Jayaraman wrote:
>> Hi Trond,
>>
>> Do you think this patch is OK? Can this be considered for merging?
>>
>> Thanks,
>>
>>>>>> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
>>>>>> index 90f292b..13d6a00 100644
>>>>>> --- a/fs/nfs/file.c
>>>>>> +++ b/fs/nfs/file.c
>>>>>> @@ -47,6 +47,9 @@ static ssize_t nfs_file_splice_read(struct file *filp, loff_t *ppos,
>>>>>> size_t count, unsigned int flags);
>>>>>> static ssize_t nfs_file_read(struct kiocb *, const struct iovec *iov,
>>>>>> unsigned long nr_segs, loff_t pos);
>>>>>> +static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
>>>>>> + struct file *filp, loff_t *ppos,
>>>>>> + size_t count, unsigned int flags);
>>>>>> static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
>>>>>> unsigned long nr_segs, loff_t pos);
>>>>>> static int nfs_file_flush(struct file *, fl_owner_t id);
>>>>>> @@ -76,6 +79,7 @@ const struct file_operations nfs_file_operations = {
>>>>>> .lock = nfs_lock,
>>>>>> .flock = nfs_flock,
>>>>>> .splice_read = nfs_file_splice_read,
>>>>>> + .splice_write = nfs_file_splice_write,
>>>>>> .check_flags = nfs_check_flags,
>>>>>> .setlease = nfs_setlease,
>>>>>> };
>>>>>> @@ -550,6 +554,26 @@ out_swapfile:
>>>>>> goto out;
>>>>>> }
>>>>>>
>>>>>> +static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
>>>>>> + struct file *filp, loff_t *ppos,
>>>>>> + size_t count, unsigned int flags)
>>>>>> +{
>>>>>> + struct dentry *dentry = filp->f_path.dentry;
>>>>>> + struct inode *inode = dentry->d_inode;
>>>>>> +
>>>>>> + dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
>>>>>> + dentry->d_parent->d_name.name, dentry->d_name.name,
>>>>>> + (unsigned long) count, (unsigned long long) *ppos);
>>>>>> +
>>>>>> + if (IS_SWAPFILE(inode)) {
>>>>>> + printk(KERN_INFO "NFS: attempt to write to active swap"
>>>>>> + "file!\n");
>>>>>> + return -EBUSY;
>>>>>> + }
>
> I don't know that we really need this. We should sweep through the NFS
> code and kill all those IS_SWAPFILE() thingys. Or at least #define
> IS_SWAPFILE(a) (0)
> ...
Hmm.. I'm not sure whether we should kill them now. I think originally,
these were added keeping in mind the future NFS swap support. Given that
the recent work from Peterz Zilstra on "Swap over NFS" and multiple
iterations/review on the same, I think those patches will eventually get
merged sooner or later. Perhaps, it's a good idea to #define
IS_SWAPFILE(a) 0 than killing them entirely..?
Thanks,
>>>>>> +
>>>>>> + return generic_file_splice_write(pipe, filp, ppos, count, flags);
>>>>>> +}
>>>>>> +
>>>>>> static int do_getlk(struct file *filp, int cmd, struct file_lock *fl)
>>>>>> {
>>>>>> struct inode *inode = filp->f_mapping->host;
>>>>>>
>
> Otherwise it looks fine...
>
>
>
--
Suresh Jayaraman
On Mon, 2009-04-20 at 18:08 +0530, Suresh Jayaraman wrote:
> Trond Myklebust wrote:
> > On Mon, 2009-04-20 at 11:09 +0530, Suresh Jayaraman wrote:
> >> Hi Trond,
> >>
> >> Do you think this patch is OK? Can this be considered for merging?
> >>
> >> Thanks,
> >>
> >>>>>> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> >>>>>> index 90f292b..13d6a00 100644
> >>>>>> --- a/fs/nfs/file.c
> >>>>>> +++ b/fs/nfs/file.c
> >>>>>> @@ -47,6 +47,9 @@ static ssize_t nfs_file_splice_read(struct file *filp, loff_t *ppos,
> >>>>>> size_t count, unsigned int flags);
> >>>>>> static ssize_t nfs_file_read(struct kiocb *, const struct iovec *iov,
> >>>>>> unsigned long nr_segs, loff_t pos);
> >>>>>> +static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
> >>>>>> + struct file *filp, loff_t *ppos,
> >>>>>> + size_t count, unsigned int flags);
> >>>>>> static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
> >>>>>> unsigned long nr_segs, loff_t pos);
> >>>>>> static int nfs_file_flush(struct file *, fl_owner_t id);
> >>>>>> @@ -76,6 +79,7 @@ const struct file_operations nfs_file_operations = {
> >>>>>> .lock = nfs_lock,
> >>>>>> .flock = nfs_flock,
> >>>>>> .splice_read = nfs_file_splice_read,
> >>>>>> + .splice_write = nfs_file_splice_write,
> >>>>>> .check_flags = nfs_check_flags,
> >>>>>> .setlease = nfs_setlease,
> >>>>>> };
> >>>>>> @@ -550,6 +554,26 @@ out_swapfile:
> >>>>>> goto out;
> >>>>>> }
> >>>>>>
> >>>>>> +static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
> >>>>>> + struct file *filp, loff_t *ppos,
> >>>>>> + size_t count, unsigned int flags)
> >>>>>> +{
> >>>>>> + struct dentry *dentry = filp->f_path.dentry;
> >>>>>> + struct inode *inode = dentry->d_inode;
> >>>>>> +
> >>>>>> + dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
> >>>>>> + dentry->d_parent->d_name.name, dentry->d_name.name,
> >>>>>> + (unsigned long) count, (unsigned long long) *ppos);
> >>>>>> +
> >>>>>> + if (IS_SWAPFILE(inode)) {
> >>>>>> + printk(KERN_INFO "NFS: attempt to write to active swap"
> >>>>>> + "file!\n");
> >>>>>> + return -EBUSY;
> >>>>>> + }
> >
> > I don't know that we really need this. We should sweep through the NFS
> > code and kill all those IS_SWAPFILE() thingys. Or at least #define
> > IS_SWAPFILE(a) (0)
> > ...
>
> Hmm.. I'm not sure whether we should kill them now. I think originally,
> these were added keeping in mind the future NFS swap support. Given that
> the recent work from Peterz Zilstra on "Swap over NFS" and multiple
> iterations/review on the same, I think those patches will eventually get
> merged sooner or later. Perhaps, it's a good idea to #define
> IS_SWAPFILE(a) 0 than killing them entirely..?
Why are they needed at all? AFAICS, other filesystems check IS_SWAPFILE
when truncating a file, but don't litter their code with all these weird
checks for writing, reading, etc.
It's not as if these checks can stop a determined privileged person from
writing to the swapfile anyway. All they have to do is go to another
client or write directly to the file on the server...
Trond
Trond Myklebust wrote:
> On Mon, 2009-04-20 at 18:08 +0530, Suresh Jayaraman wrote:
>> Trond Myklebust wrote:
>>> On Mon, 2009-04-20 at 11:09 +0530, Suresh Jayaraman wrote:
>>>> Hi Trond,
>>>>
>>>> Do you think this patch is OK? Can this be considered for merging?
>>>>>>>> +
>>>>>>>> + if (IS_SWAPFILE(inode)) {
>>>>>>>> + printk(KERN_INFO "NFS: attempt to write to active swap"
>>>>>>>> + "file!\n");
>>>>>>>> + return -EBUSY;
>>>>>>>> + }
>>> I don't know that we really need this. We should sweep through the NFS
>>> code and kill all those IS_SWAPFILE() thingys. Or at least #define
>>> IS_SWAPFILE(a) (0)
>>> ...
>> Hmm.. I'm not sure whether we should kill them now. I think originally,
>> these were added keeping in mind the future NFS swap support. Given that
>> the recent work from Peterz Zilstra on "Swap over NFS" and multiple
>> iterations/review on the same, I think those patches will eventually get
>> merged sooner or later. Perhaps, it's a good idea to #define
>> IS_SWAPFILE(a) 0 than killing them entirely..?
>
> Why are they needed at all? AFAICS, other filesystems check IS_SWAPFILE
> when truncating a file, but don't litter their code with all these weird
> checks for writing, reading, etc.
Yes, except for afs other filesystems seem to check only during
truncation.
> It's not as if these checks can stop a determined privileged person from
> writing to the swapfile anyway. All they have to do is go to another
> client or write directly to the file on the server...
>
Make sense.
Here is the trimmed version. I think the dprink could be retained
for debugging issues, if any.
Signed-off-by: Suresh Jayaraman <[email protected]>
---
fs/nfs/file.c | 17 +++++++++++++++++
1 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 5a97bcf..6dfe7df 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -48,6 +48,9 @@ static ssize_t nfs_file_splice_read(struct file *filp, loff_t *ppos,
size_t count, unsigned int flags);
static ssize_t nfs_file_read(struct kiocb *, const struct iovec *iov,
unsigned long nr_segs, loff_t pos);
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+ struct file *filp, loff_t *ppos,
+ size_t count, unsigned int flags);
static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
unsigned long nr_segs, loff_t pos);
static int nfs_file_flush(struct file *, fl_owner_t id);
@@ -73,6 +76,7 @@ const struct file_operations nfs_file_operations = {
.lock = nfs_lock,
.flock = nfs_flock,
.splice_read = nfs_file_splice_read,
+ .splice_write = nfs_file_splice_write,
.check_flags = nfs_check_flags,
.setlease = nfs_setlease,
};
@@ -587,6 +591,19 @@ out_swapfile:
goto out;
}
+static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
+ struct file *filp, loff_t *ppos,
+ size_t count, unsigned int flags)
+{
+ struct dentry *dentry = filp->f_path.dentry;
+
+ dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
+ dentry->d_parent->d_name.name, dentry->d_name.name,
+ (unsigned long) count, (unsigned long long) *ppos);
+
+ return generic_file_splice_write(pipe, filp, ppos, count, flags);
+}
+
static int do_getlk(struct file *filp, int cmd, struct file_lock *fl)
{
struct inode *inode = filp->f_mapping->host;
On Mon, Apr 20, 2009 at 09:17:23PM +0530, Suresh Jayaraman wrote:
> +static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
> + struct file *filp, loff_t *ppos,
> + size_t count, unsigned int flags)
> +{
> + struct dentry *dentry = filp->f_path.dentry;
> +
> + dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
> + dentry->d_parent->d_name.name, dentry->d_name.name,
> + (unsigned long) count, (unsigned long long) *ppos);
> +
> + return generic_file_splice_write(pipe, filp, ppos, count, flags);
> +}
> +
You need all calls from nfs_file_write, too:
- most importantly the nfs_revalidate_file_size for O_APPEND
- the nfs_do_fsync for sync writes
- probably the stats increment
On Tue, 2009-04-21 at 10:48 -0400, Christoph Hellwig wrote:
> On Mon, Apr 20, 2009 at 09:17:23PM +0530, Suresh Jayaraman wrote:
> > +static ssize_t nfs_file_splice_write(struct pipe_inode_info *pipe,
> > + struct file *filp, loff_t *ppos,
> > + size_t count, unsigned int flags)
> > +{
> > + struct dentry *dentry = filp->f_path.dentry;
> > +
> > + dprintk("NFS splice_write(%s/%s, %lu@%Lu)\n",
> > + dentry->d_parent->d_name.name, dentry->d_name.name,
> > + (unsigned long) count, (unsigned long long) *ppos);
> > +
> > + return generic_file_splice_write(pipe, filp, ppos, count, flags);
> > +}
> > +
>
> You need all calls from nfs_file_write, too:
>
> - most importantly the nfs_revalidate_file_size for O_APPEND
Isn't O_APPEND illegal for splice_write()? It looks like it is from a
quick perusal of do_splice_from().
> - the nfs_do_fsync for sync writes
generic_file_splice_write() calls generic_osync_inode(), which should
ensure sync writes even with NFS.
The one thing it won't do is propagate NFS write errors back to the
caller. If we do care about this, then we should certainly test for
nfs_need_sync_write() and then call nfs_do_fsync() (see
nfs_file_write()).
> - probably the stats increment
We should talk to Chuck about this.
Cheers
Trond