| Message ID | 20211212210044.16238-1-richard@nod.at (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] NFS: Save 4 bytes when re-exporting | expand |
On Sun, 2021-12-12 at 22:00 +0100, Richard Weinberger wrote: > When re-exporting, the whole struct nfs_fh is embedded in the new > fhandle. > But we need only nfs_fh->data[], nfs_fh->size is not needed. > So skip fs_fh->size and save a full word (4 bytes). > The downside is the extra memcpy() in nfs_fh_to_dentry(). > > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > While investigating into improving NFS re-export I noticed that > we can already save 4 bytes of overhead. > I don't think we need to embedd the full struct nfs_fh and > can skip ->size. > NACK. This will break existing running clients. Any code to change the filehandle format must be accompanied with code to detect and service filehandles that are presented in the old format.
----- Ursprüngliche Mail ----- > Von: "Trond Myklebust" <trondmy@hammerspace.com> > On Sun, 2021-12-12 at 22:00 +0100, Richard Weinberger wrote: >> When re-exporting, the whole struct nfs_fh is embedded in the new >> fhandle. >> But we need only nfs_fh->data[], nfs_fh->size is not needed. >> So skip fs_fh->size and save a full word (4 bytes). >> The downside is the extra memcpy() in nfs_fh_to_dentry(). >> >> Signed-off-by: Richard Weinberger <richard@nod.at> >> --- >> While investigating into improving NFS re-export I noticed that >> we can already save 4 bytes of overhead. >> I don't think we need to embedd the full struct nfs_fh and >> can skip ->size. >> > > NACK. This will break existing running clients. Any code to change the > filehandle format must be accompanied with code to detect and service > filehandles that are presented in the old format. One possible way to distinguish between old and new formats is looking at the length of the data. 2 * XDR_UNIT = new format (without ->size). 3 * XDR_UNIT = old format. What do you think? Thanks, //richard
----- Ursprüngliche Mail ----- > Von: "richard" <richard@nod.at> > ----- Ursprüngliche Mail ----- >> Von: "Trond Myklebust" <trondmy@hammerspace.com> >> On Sun, 2021-12-12 at 22:00 +0100, Richard Weinberger wrote: >>> When re-exporting, the whole struct nfs_fh is embedded in the new >>> fhandle. >>> But we need only nfs_fh->data[], nfs_fh->size is not needed. >>> So skip fs_fh->size and save a full word (4 bytes). >>> The downside is the extra memcpy() in nfs_fh_to_dentry(). >>> >>> Signed-off-by: Richard Weinberger <richard@nod.at> >>> --- >>> While investigating into improving NFS re-export I noticed that >>> we can already save 4 bytes of overhead. >>> I don't think we need to embedd the full struct nfs_fh and >>> can skip ->size. >>> >> >> NACK. This will break existing running clients. Any code to change the >> filehandle format must be accompanied with code to detect and service >> filehandles that are presented in the old format. > > One possible way to distinguish between old and new formats > is looking at the length of the data. > 2 * XDR_UNIT = new format (without ->size). > 3 * XDR_UNIT = old format. Never mind. This won't work. Thanks, //richard
On Sun, 2021-12-12 at 22:32 +0100, Richard Weinberger wrote: > ----- Ursprüngliche Mail ----- > > Von: "Trond Myklebust" <trondmy@hammerspace.com> > > On Sun, 2021-12-12 at 22:00 +0100, Richard Weinberger wrote: > > > When re-exporting, the whole struct nfs_fh is embedded in the new > > > fhandle. > > > But we need only nfs_fh->data[], nfs_fh->size is not needed. > > > So skip fs_fh->size and save a full word (4 bytes). > > > The downside is the extra memcpy() in nfs_fh_to_dentry(). > > > > > > Signed-off-by: Richard Weinberger <richard@nod.at> > > > --- > > > While investigating into improving NFS re-export I noticed that > > > we can already save 4 bytes of overhead. > > > I don't think we need to embedd the full struct nfs_fh and > > > can skip ->size. > > > > > > > NACK. This will break existing running clients. Any code to change > > the > > filehandle format must be accompanied with code to detect and > > service > > filehandles that are presented in the old format. > > One possible way to distinguish between old and new formats > is looking at the length of the data. > 2 * XDR_UNIT = new format (without ->size). > 3 * XDR_UNIT = old format. > > What do you think? > You don't a priori know the length of the underlying filehandle or its structure. All you know is that you have n bytes of data, and it is possible that the first 2 bytes represent the size 'n-2'. However it is also possible that those 2 bytes are something else that just happens to match the value 'n-2'.
----- Ursprüngliche Mail ----- > Von: "Trond Myklebust" <trondmy@hammerspace.com> >> What do you think? >> > > You don't a priori know the length of the underlying filehandle or its > structure. All you know is that you have n bytes of data, and it is > possible that the first 2 bytes represent the size 'n-2'. However it is > also possible that those 2 bytes are something else that just happens > to match the value 'n-2'. Yes. ;-\ Our mails have crossed, after hitting send I realized my fault. Thanks, //richard
diff --git a/fs/nfs/export.c b/fs/nfs/export.c index 5f6e1b715545..5579c87106a1 100644 --- a/fs/nfs/export.c +++ b/fs/nfs/export.c @@ -22,9 +22,9 @@ enum { }; -static struct nfs_fh *nfs_exp_embedfh(__u32 *p) +static unsigned char *nfs_exp_embedfh(__u32 *p) { - return (struct nfs_fh *)(p + EMBED_FH_OFF); + return (unsigned char *)(p + EMBED_FH_OFF); } /* @@ -35,8 +35,8 @@ static int nfs_encode_fh(struct inode *inode, __u32 *p, int *max_len, struct inode *parent) { struct nfs_fh *server_fh = NFS_FH(inode); - struct nfs_fh *clnt_fh = nfs_exp_embedfh(p); - size_t fh_size = offsetof(struct nfs_fh, data) + server_fh->size; + unsigned char *raw_fh = nfs_exp_embedfh(p); + size_t fh_size = server_fh->size; int len = EMBED_FH_OFF + XDR_QUADLEN(fh_size); dprintk("%s: max fh len %d inode %p parent %p", @@ -58,7 +58,9 @@ nfs_encode_fh(struct inode *inode, __u32 *p, int *max_len, struct inode *parent) p[FILEID_LOW_OFF] = NFS_FILEID(inode); p[FILE_I_TYPE_OFF] = inode->i_mode & S_IFMT; p[len - 1] = 0; /* Padding */ - nfs_copy_fh(clnt_fh, server_fh); + + memcpy(raw_fh, server_fh->data, server_fh->size); + *max_len = len; dprintk("%s: result fh fileid %llu mode %u size %d\n", __func__, NFS_FILEID(inode), inode->i_mode, *max_len); @@ -70,18 +72,22 @@ nfs_fh_to_dentry(struct super_block *sb, struct fid *fid, int fh_len, int fh_type) { struct nfs_fattr *fattr = NULL; - struct nfs_fh *server_fh = nfs_exp_embedfh(fid->raw); - size_t fh_size = offsetof(struct nfs_fh, data) + server_fh->size; + unsigned char *raw_server_fh = nfs_exp_embedfh(fid->raw); const struct nfs_rpc_ops *rpc_ops; struct dentry *dentry; struct inode *inode; - int len = EMBED_FH_OFF + XDR_QUADLEN(fh_size); u32 *p = fid->raw; + struct nfs_fh server_fh = { 0 }; int ret; - /* NULL translates to ESTALE */ - if (fh_len < len || fh_type != len) + server_fh.size = (fh_len * XDR_UNIT) - (EMBED_FH_OFF * XDR_UNIT); + if (server_fh.size < 1) { + WARN_ON_ONCE(1); + /* NULL translates to ESTALE */ return NULL; + } + + memcpy(server_fh.data, raw_server_fh, server_fh.size); fattr = nfs_alloc_fattr_with_label(NFS_SB(sb)); if (fattr == NULL) { @@ -95,20 +101,20 @@ nfs_fh_to_dentry(struct super_block *sb, struct fid *fid, dprintk("%s: fileid %llu mode %d\n", __func__, fattr->fileid, fattr->mode); - inode = nfs_ilookup(sb, fattr, server_fh); + inode = nfs_ilookup(sb, fattr, &server_fh); if (inode) goto out_found; rpc_ops = NFS_SB(sb)->nfs_client->rpc_ops; - ret = rpc_ops->getattr(NFS_SB(sb), server_fh, fattr, NULL); + ret = rpc_ops->getattr(NFS_SB(sb), &server_fh, fattr, NULL); if (ret) { dprintk("%s: getattr failed %d\n", __func__, ret); - trace_nfs_fh_to_dentry(sb, server_fh, fattr->fileid, ret); + trace_nfs_fh_to_dentry(sb, &server_fh, fattr->fileid, ret); dentry = ERR_PTR(ret); goto out_free_fattr; } - inode = nfs_fhget(sb, server_fh, fattr); + inode = nfs_fhget(sb, &server_fh, fattr); out_found: dentry = d_obtain_alias(inode);
When re-exporting, the whole struct nfs_fh is embedded in the new fhandle. But we need only nfs_fh->data[], nfs_fh->size is not needed. So skip fs_fh->size and save a full word (4 bytes). The downside is the extra memcpy() in nfs_fh_to_dentry(). Signed-off-by: Richard Weinberger <richard@nod.at> --- While investigating into improving NFS re-export I noticed that we can already save 4 bytes of overhead. I don't think we need to embedd the full struct nfs_fh and can skip ->size. Thanks, //richard --- fs/nfs/export.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)