[PATCH 0/2] Reduce GETATTRs during direct I/O

[PATCH 0/2] Reduce GETATTRs during direct I/O

[Chuck Lever]

Hi Trond-

I mentioned this one to you last week.  I found that the NFS direct
I/O engine appears to be missing calls to nfs_fattr_init().  Please
take a look at these two patches and let me know if they look
reasonable for 2.6.34.

The problem also appears in the enterprise linux kernels.  We're still
testing this on EL4 and EL5 in specific.  We haven't found evidence so
far of incorrect behavior after applying the direct.c fix.  A bugzilla
report is forthcoming.

I haven't convinced myself that the extra GETATTR when opening an
O_DIRECT file is ever necessary.  But, I coded up a patch to your
specifications that changes O_DIRECT opens to avoid a GETATTR when
possible.

---

Chuck Lever (2):
      NFS: Don't generate a GETATTR when opening an O_DIRECT file
      NFS: Too many GETATTR and ACCESS calls after direct I/O


 fs/nfs/dir.c    |   20 ++++++++++++++++----
 fs/nfs/direct.c |    3 +++
 2 files changed, 19 insertions(+), 4 deletions(-)

-- 
Chuck Lever

[PATCH 1/2] NFS: Too many GETATTR and ACCESS calls after direct I/O

[Chuck Lever]

The cached read and write paths initialize fattr->time_start in their
setup procedures.  The value of fattr->time_start is propagated to
read_cache_jiffies by nfs_update_inode().  Subsequent calls to
nfs_attribute_timeout() will then use a good time stamp when
computing the attribute cache timeout, and squelch unneeded GETATTR
calls.

Since the direct I/O paths erroneously leave the inode's
fattr->time_start field set to zero, read_cache_jiffies for that inode
is set to zero after any direct read or write operation.  This
triggers an otw GETATTR or ACCESS call to update the file's attribute
and access caches properly, even when the NFS READ or WRITE replies
have usable post-op attributes.

Make sure the direct read and write setup code performs the same fattr
initialization as the cached I/O paths to prevent unnecessary GETATTR
calls.

This was likely introduced by commit 0e574af1 in 2.6.15, which appears
to add new nfs_fattr_init() call sites in the cached read and write
paths, but not in the equivalent places in fs/nfs/direct.c.  A
subsequent commit in the same series, 33801147, introduces the
fattr->time_start field.

Interestingly, the direct write reschedule path already has a call to
nfs_fattr_init() in the right place.

Reported-by: Quentin Barnes <qbarnes@yahoo-inc.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Cc: stable@kernel.org
---

 fs/nfs/direct.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index e1d415e..0d28982 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -342,6 +342,7 @@ static ssize_t nfs_direct_read_schedule_segment(struct nfs_direct_req *dreq,
 		data->res.fattr = &data->fattr;
 		data->res.eof = 0;
 		data->res.count = bytes;
+		nfs_fattr_init(&data->fattr);
 		msg.rpc_argp = &data->args;
 		msg.rpc_resp = &data->res;
 
@@ -575,6 +576,7 @@ static void nfs_direct_commit_schedule(struct nfs_direct_req *dreq)
 	data->res.count = 0;
 	data->res.fattr = &data->fattr;
 	data->res.verf = &data->verf;
+	nfs_fattr_init(&data->fattr);
 
 	NFS_PROTO(data->inode)->commit_setup(data, &msg);
 
@@ -766,6 +768,7 @@ static ssize_t nfs_direct_write_schedule_segment(struct nfs_direct_req *dreq,
 		data->res.fattr = &data->fattr;
 		data->res.count = bytes;
 		data->res.verf = &data->verf;
+		nfs_fattr_init(&data->fattr);
 
 		task_setup_data.task = &data->task;
 		task_setup_data.callback_data = data;

[PATCH 2/2] NFS: Don't generate a GETATTR when opening an O_DIRECT file

[Chuck Lever]

Close-to-open isn't needed for O_DIRECT files, since their data is
never cached.  So if their attribute cache hasn't expired, skip the
GETATTR.

Note that fattr->time_start is required to be set correctly by the NFS
direct I/O paths in order for this patch to work properly.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfs/dir.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 3c7f03b..367302b 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -709,6 +709,12 @@ static int nfs_is_exclusive_create(struct inode *dir, struct nameidata *nd)
 	return nd && nfs_lookup_check_intent(nd, LOOKUP_EXCL);
 }
 
+static bool nfs_open_odirect(struct nameidata *nd)
+{
+	struct file *filp = nd->intent.open.file;
+	return filp && (filp->f_flags & O_DIRECT);
+}
+
 /*
  * Inode and filehandle revalidation for lookups.
  *
@@ -716,6 +722,9 @@ static int nfs_is_exclusive_create(struct inode *dir, struct nameidata *nd)
  * or if the intent information indicates that we're about to open this
  * particular file and the "nocto" mount flag is not set.
  *
+ * O_DIRECT files have no locally cached data, so close-to-open checking
+ * isn't necessary.  However, their attributes must be revalidated if the
+ * cache has expired, mainly to catch the case where actimeo is zero.
  */
 static inline
 int nfs_lookup_verify_inode(struct inode *inode, struct nameidata *nd)
@@ -730,12 +739,15 @@ int nfs_lookup_verify_inode(struct inode *inode, struct nameidata *nd)
 			goto out_force;
 		/* This is an open(2) */
 		if (nfs_lookup_check_intent(nd, LOOKUP_OPEN) != 0 &&
-				!(server->flags & NFS_MOUNT_NOCTO) &&
-				(S_ISREG(inode->i_mode) ||
-				 S_ISDIR(inode->i_mode)))
-			goto out_force;
+		    (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
+			if (nfs_open_odirect(nd))
+				goto out_conditional;
+			if (!(server->flags & NFS_MOUNT_NOCTO))
+				goto out_force;
+		}
 		return 0;
 	}
+out_conditional:
 	return nfs_revalidate_inode(server, inode);
 out_force:
 	return __nfs_revalidate_inode(server, inode);

Re: [PATCH 2/2] NFS: Don't generate a GETATTR when opening an O_DIRECT file

[Chuck Lever]

On 02/11/2010 02:09 PM, Chuck Lever wrote:
> Close-to-open isn't needed for O_DIRECT files, since their data is
> never cached.  So if their attribute cache hasn't expired, skip the
> GETATTR.
>
> Note that fattr->time_start is required to be set correctly by the NFS
> direct I/O paths in order for this patch to work properly.
>
> Signed-off-by: Chuck Lever<chuck.lever@oracle.com>
> ---
>
>   fs/nfs/dir.c |   20 ++++++++++++++++----
>   1 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 3c7f03b..367302b 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -709,6 +709,12 @@ static int nfs_is_exclusive_create(struct inode *dir, struct nameidata *nd)
>   	return nd&&  nfs_lookup_check_intent(nd, LOOKUP_EXCL);
>   }
>
> +static bool nfs_open_odirect(struct nameidata *nd)
> +{
> +	struct file *filp = nd->intent.open.file;
> +	return filp&&  (filp->f_flags&  O_DIRECT);
> +}
> +
>   /*
>    * Inode and filehandle revalidation for lookups.
>    *
> @@ -716,6 +722,9 @@ static int nfs_is_exclusive_create(struct inode *dir, struct nameidata *nd)
>    * or if the intent information indicates that we're about to open this
>    * particular file and the "nocto" mount flag is not set.
>    *
> + * O_DIRECT files have no locally cached data, so close-to-open checking
> + * isn't necessary.  However, their attributes must be revalidated if the
> + * cache has expired, mainly to catch the case where actimeo is zero.

Missing word...

+ * attribute cache has expired,

>    */
>   static inline
>   int nfs_lookup_verify_inode(struct inode *inode, struct nameidata *nd)
> @@ -730,12 +739,15 @@ int nfs_lookup_verify_inode(struct inode *inode, struct nameidata *nd)
>   			goto out_force;
>   		/* This is an open(2) */
>   		if (nfs_lookup_check_intent(nd, LOOKUP_OPEN) != 0&&
> -				!(server->flags&  NFS_MOUNT_NOCTO)&&
> -				(S_ISREG(inode->i_mode) ||
> -				 S_ISDIR(inode->i_mode)))
> -			goto out_force;
> +		    (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
> +			if (nfs_open_odirect(nd))
> +				goto out_conditional;
> +			if (!(server->flags&  NFS_MOUNT_NOCTO))
> +				goto out_force;
> +		}
>   		return 0;
>   	}
> +out_conditional:
>   	return nfs_revalidate_inode(server, inode);
>   out_force:
>   	return __nfs_revalidate_inode(server, inode);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 2/2] NFS: Don't generate a GETATTR when opening an O_DIRECT file

[Trond Myklebust]

On Thu, 2010-02-11 at 14:09 -0500, Chuck Lever wrote: 
> Close-to-open isn't needed for O_DIRECT files, since their data is
> never cached.  So if their attribute cache hasn't expired, skip the
> GETATTR.

Don't we still want to ensure that the access cache is still valid?

Cheers
  Trond

Re: [PATCH 2/2] NFS: Don't generate a GETATTR when opening an O_DIRECT file

[Chuck Lever]

On 02/11/2010 02:14 PM, Trond Myklebust wrote:
> On Thu, 2010-02-11 at 14:09 -0500, Chuck Lever wrote:
>> Close-to-open isn't needed for O_DIRECT files, since their data is
>> never cached.  So if their attribute cache hasn't expired, skip the
>> GETATTR.
>
> Don't we still want to ensure that the access cache is still valid?

Would it be reasonable/feasible to squelch the GETATTR but force an 
ACCESS call from nfs_permission?

-- 
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 2/2] NFS: Don't generate a GETATTR when opening an O_DIRECT file

[Trond Myklebust]

On Thu, 2010-02-11 at 14:20 -0500, Chuck Lever wrote: 
> On 02/11/2010 02:14 PM, Trond Myklebust wrote:
> > On Thu, 2010-02-11 at 14:09 -0500, Chuck Lever wrote:
> >> Close-to-open isn't needed for O_DIRECT files, since their data is
> >> never cached.  So if their attribute cache hasn't expired, skip the
> >> GETATTR.
> >
> > Don't we still want to ensure that the access cache is still valid?
> 
> Would it be reasonable/feasible to squelch the GETATTR but force an 
> ACCESS call from nfs_permission?

As long as the ACCESS call returns post-op attributes, then it is
reasonable to do this for NFSv3 (or for NFSv4 opendir()) in all cases.

I used to have patches for this, but was never able to show that the
resulting total number of GETATTR+ACCESS calls was much affected.

Cheers
   Trond

Re: [PATCH 2/2] NFS: Don't generate a GETATTR when opening an O_DIRECT file

[Chuck Lever]

On 02/11/2010 02:34 PM, Trond Myklebust wrote:
> On Thu, 2010-02-11 at 14:20 -0500, Chuck Lever wrote:
>> On 02/11/2010 02:14 PM, Trond Myklebust wrote:
>>> On Thu, 2010-02-11 at 14:09 -0500, Chuck Lever wrote:
>>>> Close-to-open isn't needed for O_DIRECT files, since their data is
>>>> never cached.  So if their attribute cache hasn't expired, skip the
>>>> GETATTR.
>>>
>>> Don't we still want to ensure that the access cache is still valid?
>>
>> Would it be reasonable/feasible to squelch the GETATTR but force an
>> ACCESS call from nfs_permission?
>
> As long as the ACCESS call returns post-op attributes, then it is
> reasonable to do this for NFSv3 (or for NFSv4 opendir()) in all cases.
>
> I used to have patches for this, but was never able to show that the
> resulting total number of GETATTR+ACCESS calls was much affected.

It probably doesn't make a whole lot of difference in this case either, 
then.  The client would generate a GETATTR which effectively refreshes 
both the attribute cache and the access cache, or an ACCESS that does 
almost the same.

The previous patch probably fixes the (by far) largest part of the 
request overage here.

Does it make sense to drop this patch?

-- 
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 2/2] NFS: Don't generate a GETATTR when opening an O_DIRECT file

[Trond Myklebust]

On Thu, 2010-02-11 at 14:41 -0500, Chuck Lever wrote: 
> Does it make sense to drop this patch?

Yes, I think the gains would be minimal (although I'd be happy to be
proven wrong).

Cheers
  Trond