Re: svn commit: r212439 - head/sys/fs/nfs
On Sat, Sep 11, 2010 at 08:41:22PM -0400, Rick Macklem wrote:
> > Then, fid_reserved is no more reserved ? Should we rename it ?
> >
> > Comment for fid_reserved about longword alignment is wrong.
>
> Well, it's actually more broken than that.
> fid_len - Most file systems set it to the size of their variant
> of the entire structure, including the Xfid_len field.
> ZFS sets it to the size of the structure - sizeof(uint16_t)
> { presumably subtracting out the size if Xfid_len? }.
> And xfs, well, it does weird stuff with it I can't figure
> out, but it is definitely not the size of the entire struct.
>
> As such, exposing fid_len above the VOP_xxx() doesn't make much sense.
> (After my commit yesterday, nothing above the VOP_VPTOFH() uses it.)
>
> Personally, I'd lean towards a generic struct fid like...
> struct fid {
>uint8_t fid_data[MAXFIDSZ];
> };
> with MAXFIDSZ increased appropriately, but this will require changes
> to xfs and zfs, since they both set the generic fid_len.
>
> If you go with...
> struct fid {
>uint16_t fid_len;
>uint8_t fid_data[MAXFIDSZ];
> };
> then the hash functions in the two NFS servers need to be changed
> (they assume 32bit alignment of fid_data), but they should be fixed
> anyhow, since they mostly hash to 0 for ZFS at this time. (From what
> I see ZFS file handles looking like.)
>
> Or, you could just rename fid_reserved to fid_pad and not worry about it.
>
> Maybe the ZFS folks could decide what they would prefer? rick
Let at least rename the field. And I propose the name like fid_data0
or similar, not the fid_pad, to signify that it is used.
pgp1Tc1u0ve3V.pgp
Description: PGP signature
Re: svn commit: r212439 - head/sys/fs/nfs
On Sun, Sep 12, 2010 at 12:41 AM, Rick Macklem wrote:
>> Then, fid_reserved is no more reserved ? Should we rename it ?
>>
>> Comment for fid_reserved about longword alignment is wrong.
>
> Well, it's actually more broken than that.
> fid_len - Most file systems set it to the size of their variant
> of the entire structure, including the Xfid_len field.
> ZFS sets it to the size of the structure - sizeof(uint16_t)
> { presumably subtracting out the size if Xfid_len? }.
> And xfs, well, it does weird stuff with it I can't figure
> out, but it is definitely not the size of the entire struct.
>
> As such, exposing fid_len above the VOP_xxx() doesn't make much sense.
> (After my commit yesterday, nothing above the VOP_VPTOFH() uses it.)
>
> Personally, I'd lean towards a generic struct fid like...
> struct fid {
> uint8_t fid_data[MAXFIDSZ];
> };
Isilon would love a generic struct like this, as to fit our filesystem
we had to make such a change locally anyways. :-)
Cheers,
matthew
> with MAXFIDSZ increased appropriately, but this will require changes
> to xfs and zfs, since they both set the generic fid_len.
>
> If you go with...
> struct fid {
> uint16_t fid_len;
> uint8_t fid_data[MAXFIDSZ];
> };
> then the hash functions in the two NFS servers need to be changed
> (they assume 32bit alignment of fid_data), but they should be fixed
> anyhow, since they mostly hash to 0 for ZFS at this time. (From what
> I see ZFS file handles looking like.)
>
> Or, you could just rename fid_reserved to fid_pad and not worry about it.
>
> Maybe the ZFS folks could decide what they would prefer? rick
>
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r212439 - head/sys/fs/nfs
> Then, fid_reserved is no more reserved ? Should we rename it ?
>
> Comment for fid_reserved about longword alignment is wrong.
Well, it's actually more broken than that.
fid_len - Most file systems set it to the size of their variant
of the entire structure, including the Xfid_len field.
ZFS sets it to the size of the structure - sizeof(uint16_t)
{ presumably subtracting out the size if Xfid_len? }.
And xfs, well, it does weird stuff with it I can't figure
out, but it is definitely not the size of the entire struct.
As such, exposing fid_len above the VOP_xxx() doesn't make much sense.
(After my commit yesterday, nothing above the VOP_VPTOFH() uses it.)
Personally, I'd lean towards a generic struct fid like...
struct fid {
uint8_t fid_data[MAXFIDSZ];
};
with MAXFIDSZ increased appropriately, but this will require changes
to xfs and zfs, since they both set the generic fid_len.
If you go with...
struct fid {
uint16_t fid_len;
uint8_t fid_data[MAXFIDSZ];
};
then the hash functions in the two NFS servers need to be changed
(they assume 32bit alignment of fid_data), but they should be fixed
anyhow, since they mostly hash to 0 for ZFS at this time. (From what
I see ZFS file handles looking like.)
Or, you could just rename fid_reserved to fid_pad and not worry about it.
Maybe the ZFS folks could decide what they would prefer? rick
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r212439 - head/sys/fs/nfs
On Fri, Sep 10, 2010 at 11:18:45PM +, Rick Macklem wrote:
> Author: rmacklem
> Date: Fri Sep 10 23:18:45 2010
> New Revision: 212439
> URL: http://svn.freebsd.org/changeset/base/212439
>
> Log:
> Fix the NFSVNO_CMPFH() macro in the experimental NFS server so
> that it works correctly for ZFS file handles. It is possible to
> have two ZFS file handles that differ only in the bytes in the
> fid_reserved field of the generic "struct fid" and comparing the
> bytes in fid_data didn't catch this case. This patch changes the
> macro to compare all bytes of "struct fid".
>
> Tested by: gull at gull.us
> MFC after: 2 weeks
>
> Modified:
> head/sys/fs/nfs/nfsdport.h
>
> Modified: head/sys/fs/nfs/nfsdport.h
> ==
> --- head/sys/fs/nfs/nfsdport.hFri Sep 10 23:15:05 2010
> (r212438)
> +++ head/sys/fs/nfs/nfsdport.hFri Sep 10 23:18:45 2010
> (r212439)
> @@ -70,8 +70,7 @@ struct nfsexstuff {
> #define NFSVNO_CMPFH(f1, f2)
> \
> ((f1)->fh_fsid.val[0] == (f2)->fh_fsid.val[0] && \
> (f1)->fh_fsid.val[1] == (f2)->fh_fsid.val[1] && \
> - !bcmp((f1)->fh_fid.fid_data, (f2)->fh_fid.fid_data, \
> -(f1)->fh_fid.fid_len))
> + bcmp(&(f1)->fh_fid, &(f2)->fh_fid, sizeof(struct fid)) == 0)
>
> #define NFSLOCKHASH(f)
> \
> (&nfslockhash[(*((u_int32_t *)((f)->fh_fid.fid_data))) %
> NFSLOCKHASHSIZE])
Then, fid_reserved is no more reserved ? Should we rename it ?
Comment for fid_reserved about longword alignment is wrong.
pgpSfVd6iyqBh.pgp
Description: PGP signature
svn commit: r212439 - head/sys/fs/nfs
Author: rmacklem
Date: Fri Sep 10 23:18:45 2010
New Revision: 212439
URL: http://svn.freebsd.org/changeset/base/212439
Log:
Fix the NFSVNO_CMPFH() macro in the experimental NFS server so
that it works correctly for ZFS file handles. It is possible to
have two ZFS file handles that differ only in the bytes in the
fid_reserved field of the generic "struct fid" and comparing the
bytes in fid_data didn't catch this case. This patch changes the
macro to compare all bytes of "struct fid".
Tested by:gull at gull.us
MFC after:2 weeks
Modified:
head/sys/fs/nfs/nfsdport.h
Modified: head/sys/fs/nfs/nfsdport.h
==
--- head/sys/fs/nfs/nfsdport.h Fri Sep 10 23:15:05 2010(r212438)
+++ head/sys/fs/nfs/nfsdport.h Fri Sep 10 23:18:45 2010(r212439)
@@ -70,8 +70,7 @@ struct nfsexstuff {
#defineNFSVNO_CMPFH(f1, f2)
\
((f1)->fh_fsid.val[0] == (f2)->fh_fsid.val[0] && \
(f1)->fh_fsid.val[1] == (f2)->fh_fsid.val[1] && \
- !bcmp((f1)->fh_fid.fid_data, (f2)->fh_fid.fid_data, \
-(f1)->fh_fid.fid_len))
+ bcmp(&(f1)->fh_fid, &(f2)->fh_fid, sizeof(struct fid)) == 0)
#defineNFSLOCKHASH(f)
\
(&nfslockhash[(*((u_int32_t *)((f)->fh_fid.fid_data))) %
NFSLOCKHASHSIZE])
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
