On Jan. 22, 2008, 20:10 +0200, Trond Myklebust <[EMAIL PROTECTED]> wrote:
> On Tue, 2008-01-22 at 11:58 -0500, Chuck Lever wrote:
>> Hi Benny-
>>
>> On Jan 22, 2008, at 10:37 AM, Benny Halevy wrote:
>>> On Jan. 22, 2008, 16:50 +0200, Trond Myklebust
>>> <[EMAIL PROTECTED]> wrote:
>>>> On Tue, 2008-01-22 at 16:28 +0200, Benny Halevy wrote:
>>>>> Otherwise e.g., NFS_SERVER(&nfsi->vfs_inode) does not compile.
>>>>>
>>>>> Signed-off-by: Benny Halevy <[EMAIL PROTECTED]>
>>>>> ---
>>>>> include/linux/nfs_fs.h | 4 ++--
>>>>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>>>>> index 0477a4c..5a5d3fe 100644
>>>>> --- a/include/linux/nfs_fs.h
>>>>> +++ b/include/linux/nfs_fs.h
>>>>> @@ -221,10 +221,10 @@ static inline struct nfs_inode *NFS_I
>>>>> (struct inode *inode)
>>>>> {
>>>>> return container_of(inode, struct nfs_inode, vfs_inode);
>>>>> }
>>>>> -#define NFS_SB(s) ((struct nfs_server *)(s->s_fs_info))
>>>>> +#define NFS_SB(s) ((struct nfs_server *)((s)->s_fs_info))
>>>>>
>>>>> #define NFS_FH(inode) (&NFS_I(inode)->fh)
>>>>> -#define NFS_SERVER(inode) (NFS_SB(inode->i_sb))
>>>>> +#define NFS_SERVER(inode) (NFS_SB((inode)->i_sb))
>>>>> #define NFS_CLIENT(inode) (NFS_SERVER(inode)->client)
>>>>> #define NFS_PROTO(inode) (NFS_SERVER(inode)->nfs_client->rpc_ops)
>>>>> #define NFS_COOKIEVERF(inode) (NFS_I(inode)->cookieverf)
>>>> They should really be converted into inlined functions.
>>>>
>>>> Cheers
>>>> Trond
>>> Agreed. How about the following:
>>> ---
>>> [PATCH] nfs: convert NFS_*(inode) helpers to static inline
>>>
>>> Signed-off-by: Benny Halevy <[EMAIL PROTECTED]>
>>> ---
>>> (Patch passes all connectathon tests)
>>>
>>> fs/nfs/dir.c | 8 ++--
>>> fs/nfs/inode.c | 8 ++--
>>> fs/nfs/read.c | 2 +-
>>> include/linux/nfs_fs.h | 78 ++++++++++++++++++++++++++++++++++++
>>> +----------
>>> 4 files changed, 70 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
>>> index f697b5c..7b64c22 100644
>>> --- a/fs/nfs/dir.c
>>> +++ b/fs/nfs/dir.c
>>> @@ -192,7 +192,7 @@ int nfs_readdir_filler(nfs_readdir_descriptor_t
>>> *desc, struct page *page)
>>> /* We requested READDIRPLUS, but the server doesn't grok it */
>>> if (error == -ENOTSUPP && desc->plus) {
>>> NFS_SERVER(inode)->caps &= ~NFS_CAP_READDIRPLUS;
>>> - clear_bit(NFS_INO_ADVISE_RDPLUS, &NFS_FLAGS(inode));
>>> + clear_bit(NFS_INO_ADVISE_RDPLUS, NFS_FLAGSP(inode));
>> Since you already have NFS_USE_READDIRPLUS defined below, maybe the
>> equivalent clear_bit functionality can also be an inlined function.
>> It is even used in more than one place.
>
> I disagree. The inlined wrapper adds nothing but obfuscation in this
> case. It would be different if you needed memory barriers, but that is
> not the case here.
>
>> I feel like NFS_FLAGSP (returning a pointer) is somewhat awkward, but
>> that's just my taste I suppose.
>
> Ideally, we should get rid of NFS_FLAGS()/NFS_FLAGSP(). That too is just
> obfuscating the code for no good reason.
I completely agree that NFS_FLAGSP is ugly.
Initially I thought of changing &NFS_FLAGS(inode) to &NFS_I(inode)->flags
and get rid of NFS_FLAGS, but I deferred to the most minimal change.
Let me know if you want me to do that.
>
>>> static void nfs_invalidate_inode(struct inode *inode)
>>> {
>>> - set_bit(NFS_INO_STALE, &NFS_FLAGS(inode));
>>> + set_bit(NFS_INO_STALE, NFS_FLAGSP(inode));
>> Likewise for NFS_INO_STALE... A separate inline for setting
>> NFS_INO_STALE might be a little nicer.
>
> Not an inline. Just convert the existing nfs_invalidate_inode() into an
> nfs_invalidate_inode_locked(), and add a version that takes the lock.
I'm not sure I follow you... All it does is setting the NFS_INO_STALE
bit and calling nfs_zap_caches_locked. What use case is there for
the unlocked case?
>
>>> nfs_zap_caches_locked(inode);
>>> }
>>>
>>> @@ -229,7 +229,7 @@ nfs_init_locked(struct inode *inode, void *opaque)
>>> struct nfs_find_desc *desc = (struct nfs_find_desc *)opaque;
>>> struct nfs_fattr *fattr = desc->fattr;
>>>
>>> - NFS_FILEID(inode) = fattr->fileid;
>>> + set_nfs_fileid(inode, fattr->fileid);
>>> nfs_copy_fh(NFS_FH(inode), desc->fh);
>>> return 0;
>>> }
>>> @@ -291,7 +291,7 @@ nfs_fhget(struct super_block *sb, struct nfs_fh
>>> *fh, struct nfs_fattr *fattr)
>>> inode->i_fop = &nfs_dir_operations;
>>> if (nfs_server_capable(inode, NFS_CAP_READDIRPLUS)
>>> && fattr->size <= NFS_LIMIT_READDIRPLUS)
>>> - set_bit(NFS_INO_ADVISE_RDPLUS,
>>> &NFS_FLAGS(inode));
>>> + set_bit(NFS_INO_ADVISE_RDPLUS,
>>> NFS_FLAGSP(inode));
>> And for setting NFS_INO_ADVISE_RDPLUS.
>
> Again, why?
The only good reason I can think of is abstracting the API to allow a different
implementation in the future, but I see little benefits as for style or
readability.
>
>>> (inode)))
>>> +static inline struct nfs_fh *NFS_FH(const struct inode *inode)
>>> +{
>>> + return &NFS_I(inode)->fh;
>>> +}
>> Since these are no longer macros, maybe we should change the case of
>> their names too. I realize NFS_USE_READDIRPLUS has set a precedent,
>> but perhaps it's an ugly one we should fix now.
>
> Changing NFS_I() would break with a common practice that is shared with
> almost all filesystems. See, for instance, EXT3_I(), REISERFS_I(),
> XFS_I(),...
>
>
> Trond
-
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
