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.
> > 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.
> > 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?
> > (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
