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 <[email protected]>
---
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);
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
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<[email protected]>
> ---
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
chuck[dot]lever[at]oracle[dot]com
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
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
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
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