On Wed, Mar 25, 2009 at 07:07:07PM +0530, Krishna Kumar wrote:
> @@ -1337,12 +1321,30 @@ nfsd_read(struct svc_rqst *rqstp, struct
> goto out;
> err = nfsd_vfs_read(rqstp, fhp, file, offset, vec, vlen, count);
> } else {
> - err = nfsd_open(rqstp, fhp, S_IFREG, NFSD_MAY_READ, &file);
> - if (err)
> - goto out;
> - err = nfsd_vfs_read(rqstp, fhp, file, offset, vec, vlen, count);
> - nfsd_close(file);
> + struct fhcache *fh;
> +
> + /* Check if this fh is cached */
> + fh = nfsd_get_fhcache(fhp->fh_handle.fh_auth[3]);
How do you know fh_auth[3] is sufficient to identify the file reliably?
This looks very fragile to me.
If the goal is to bypass rqst_exp_find(), exportfs_decode_fh() and
friends--that makes me nervous. We should figure out how to make those
lookups faster instead. Or at the very least, make sure we're keying on
the entire filehandle instead of just part of it.
--b.
> + if (fh && fh->p_filp) {
> + /* Got cached values */
> + file = fh->p_filp;
> + fhp->fh_dentry = file->f_dentry;
> + fhp->fh_export = fh->p_exp;
> + err = fh_verify(rqstp, fhp, S_IFREG, NFSD_MAY_READ);
> + } else {
> + /* Nothing in cache */
> + err = nfsd_open(rqstp, fhp, S_IFREG, NFSD_MAY_READ,
> + &file);
> + }
> +
> + if (!err)
> + err = nfsd_vfs_read(rqstp, fhp, file, offset, vec, vlen,
> + count);
> +
> + /* Update cached values if required, and clean up */
> + fh_cache_upd(fh, file, fhp->fh_export);
> }
> +
> out:
> return err;
> }
Hi Bruce,
"J. Bruce Fields" <[email protected]> wrote on 04/24/2009 04:09:09 AM:
> > Keying on the entire filehandle seems reasonable, but I don't think it
is
> > required as auth[3] seems to be allocated memory which is unique in a
> > system,
>
> You lost me. Where do you see auth[3] getting encoded, and what do you
> mean by saying it's "allocated memory which is unique in a system"?
>
> There are a lot of different filehandle encoding options. I lose track
> of them myself....
Sorry, I am wrong, I was thinking of &auth[3] and wrote too fast. I am
testing
using your suggestion hashing on the full filehandle, something like:
{
__u32 a = auth[0], b = auth[1], c = auth[2], d = auth[3];
hash = jhash_3words(a, b, jhash_2words(c, d, 0), 0xfeedbeef) &
FHPARM_HASH_MASK;
...
/*
* Matching check uses something like:
* if (fh->p_auth1 == a && fh->p_auth2 == b && fh->p_auth3 == c &&
fh->p_auth4 == d)
*/
}
Is what you had in mind? I am testing some more with this, so far I get
different values for different files and filesystems.
I am not sure if there is an easier way to do a hash and get the unique
file
associated with the filehandle, this part of the code is very complicated.
> > just that we don't use it for locating the file currently and I was
> > proposing
> > that we do. Please correct me if this is wrong, or whether a better way
is
> > possible.
> >
> > > This patch doesn't change anything--it only adds new lines. If
you're
> > > trying to make minimal changes, that's admirable, but break up the
> > > patches in such a way that each patch shows that minimal change.
> >
> > To make each revision compile cleanly, I had to add the infrastructure
and
> > then delete the old one. So it looks like new code. The entire function
and
> > data structures are re-written so it is difficult to break into
individual
> > components where each will compile clean.
>
> Breaking up a complex change into logical individual steps, each of
> which compile and run cleanly, is sometimes tricky. So it may be that
> there *is* some way to do it in this case, but that you just haven't
> found it yet.
>
> That said, it could be, as you say, that this requires just replacing
> everything. In that case, please just start over from scratch and don't
> feel bound to keep stuff from the old implementation that you don't
> understand.
OK.
thanks,
- KK
On Fri, Apr 24, 2009 at 09:47:57PM +0530, Krishna Kumar2 wrote:
> Hi Bruce,
>
> "J. Bruce Fields" <[email protected]> wrote on 04/24/2009 04:09:09 AM:
>
> > > Keying on the entire filehandle seems reasonable, but I don't think it
> is
> > > required as auth[3] seems to be allocated memory which is unique in a
> > > system,
> >
> > You lost me. Where do you see auth[3] getting encoded, and what do you
> > mean by saying it's "allocated memory which is unique in a system"?
> >
> > There are a lot of different filehandle encoding options. I lose track
> > of them myself....
>
> Sorry, I am wrong, I was thinking of &auth[3] and wrote too fast. I am
> testing
> using your suggestion hashing on the full filehandle, something like:
>
> {
> __u32 a = auth[0], b = auth[1], c = auth[2], d = auth[3];
> hash = jhash_3words(a, b, jhash_2words(c, d, 0), 0xfeedbeef) &
> FHPARM_HASH_MASK;
> ...
> /*
> * Matching check uses something like:
> * if (fh->p_auth1 == a && fh->p_auth2 == b && fh->p_auth3 == c &&
> fh->p_auth4 == d)
> */
> }
>
> Is what you had in mind? I am testing some more with this, so far I get
> different values for different files and filesystems.
>
> I am not sure if there is an easier way to do a hash and get the unique
> file
> associated with the filehandle, this part of the code is very complicated.
Why not just do a hash on the entire filehandle, however long it may be?
(Cc'ing Greg since he says he had some patches which did something
similar, and perhaps he could offer a suggestion.)
--b.
Hi Bruce,
"J. Bruce Fields" <[email protected]> wrote on 04/24/2009 09:53:21 PM:
> > {
> > __u32 a = auth[0], b = auth[1], c = auth[2], d = auth[3];
> > hash = jhash_3words(a, b, jhash_2words(c, d, 0), 0xfeedbeef) &
> > FHPARM_HASH_MASK;
> > ...
> > /*
> > * Matching check uses something like:
> > * if (fh->p_auth1 == a && fh->p_auth2 == b && fh->p_auth3 == c
&&
> > fh->p_auth4 == d)
> > */
> > }
> >
> > Is what you had in mind? I am testing some more with this, so far I get
> > different values for different files and filesystems.
> >
> > I am not sure if there is an easier way to do a hash and get the unique
> > file
> > associated with the filehandle, this part of the code is very
complicated.
>
> Why not just do a hash on the entire filehandle, however long it may be?
I am not sure how many numbers to hash on, usually the first 4 numbers are
the
ino, inode generation, parent inode, parent inode generation, etc, and is a
unique match. Or filesystems can have their own encode handlers but store
similar stuff in these indices. I guess a memcmp could also be done if I
know
the length of the auth being used.
> (Cc'ing Greg since he says he had some patches which did something
> similar, and perhaps he could offer a suggestion.)
OK, will wait for response from Greg.
thanks,
- KK
On Fri, Apr 24, 2009 at 10:28:18PM +0530, Krishna Kumar2 wrote:
> Hi Bruce,
>
> "J. Bruce Fields" <[email protected]> wrote on 04/24/2009 09:53:21 PM:
>
> > > {
> > > __u32 a = auth[0], b = auth[1], c = auth[2], d = auth[3];
> > > hash = jhash_3words(a, b, jhash_2words(c, d, 0), 0xfeedbeef) &
> > > FHPARM_HASH_MASK;
> > > ...
> > > /*
> > > * Matching check uses something like:
> > > * if (fh->p_auth1 == a && fh->p_auth2 == b && fh->p_auth3 == c
> &&
> > > fh->p_auth4 == d)
> > > */
> > > }
> > >
> > > Is what you had in mind? I am testing some more with this, so far I get
> > > different values for different files and filesystems.
> > >
> > > I am not sure if there is an easier way to do a hash and get the unique
> > > file
> > > associated with the filehandle, this part of the code is very
> complicated.
> >
> > Why not just do a hash on the entire filehandle, however long it may be?
>
> I am not sure how many numbers to hash on, usually the first 4 numbers are
> the
> ino, inode generation, parent inode, parent inode generation, etc, and is a
> unique match. Or filesystems can have their own encode handlers but store
> similar stuff in these indices. I guess a memcmp could also be done if I
> know
> the length of the auth being used.
Why not use the whole thing?:
fh1->fh_size == fh2->fh_size
&& memcmp(fh1->fh_base, fh2->fh_base, fh1->fh_size) == 0
--b.
>
> > (Cc'ing Greg since he says he had some patches which did something
> > similar, and perhaps he could offer a suggestion.)
>
> OK, will wait for response from Greg.
>
> thanks,
>
> - KK
>
"J. Bruce Fields" <[email protected]> wrote on 04/25/2009 12:55:06 AM:
> Why not use the whole thing?:
>
> fh1->fh_size == fh2->fh_size
> && memcmp(fh1->fh_base, fh2->fh_base, fh1->fh_size) == 0
OK - so I just add a knfsd_fh in the cache and compare fh's. That
sounds good, let me test and see what is the result.
thanks,
- KK
Hi Bruce,
(combining all three of your mails into one response for easier tracking)
"J. Bruce Fields" <[email protected]> wrote on 04/23/2009 01:35:53 AM:
> How do you know fh_auth[3] is sufficient to identify the file reliably?
> This looks very fragile to me.
>
> If the goal is to bypass rqst_exp_find(), exportfs_decode_fh() and
> friends--that makes me nervous. We should figure out how to make those
> lookups faster instead. Or at the very least, make sure we're keying on
> the entire filehandle instead of just part of it.
To understand how filehandles works, I was looking at nfsd_set_fh_dentry,
here fid is fh->auth[2] (since type is FSID_DEV). However, the unique FH
value is stored in fh->auth[3], while auth[0]/[1] contains the maj/min and
ino. fh->auth[3] is unique for different files and across file systems, but
it is not actually used to find the file (only the exp) - the stored
dev/ino
(auth[0] and auth[1]) are used to find the dentry by calling the underlying
filesystem (exportfs_decode_fh).
Keying on the entire filehandle seems reasonable, but I don't think it is
required as auth[3] seems to be allocated memory which is unique in a
system,
just that we don't use it for locating the file currently and I was
proposing
that we do. Please correct me if this is wrong, or whether a better way is
possible.
> This patch doesn't change anything--it only adds new lines. If you're
> trying to make minimal changes, that's admirable, but break up the
> patches in such a way that each patch shows that minimal change.
To make each revision compile cleanly, I had to add the infrastructure and
then delete the old one. So it looks like new code. The entire function and
data structures are re-written so it is difficult to break into individual
components where each will compile clean.
> By "noisy" I just mean that they look like there's a lot of randomness.
> So, yes, I'm fine with a subset, but I'm curious what the variance is on
> repeated runs of each test.
Sure, I will try to do this over the weekend and submit.
thanks,
- KK
On Thu, Apr 23, 2009 at 09:25:34PM +0530, Krishna Kumar2 wrote:
> Hi Bruce,
>
> (combining all three of your mails into one response for easier tracking)
>
> "J. Bruce Fields" <[email protected]> wrote on 04/23/2009 01:35:53 AM:
>
> > How do you know fh_auth[3] is sufficient to identify the file reliably?
> > This looks very fragile to me.
> >
> > If the goal is to bypass rqst_exp_find(), exportfs_decode_fh() and
> > friends--that makes me nervous. We should figure out how to make those
> > lookups faster instead. Or at the very least, make sure we're keying on
> > the entire filehandle instead of just part of it.
>
> To understand how filehandles works, I was looking at nfsd_set_fh_dentry,
> here fid is fh->auth[2] (since type is FSID_DEV). However, the unique FH
> value is stored in fh->auth[3], while auth[0]/[1] contains the maj/min and
> ino. fh->auth[3] is unique for different files and across file systems, but
> it is not actually used to find the file (only the exp) - the stored
> dev/ino
> (auth[0] and auth[1]) are used to find the dentry by calling the underlying
> filesystem (exportfs_decode_fh).
>
> Keying on the entire filehandle seems reasonable, but I don't think it is
> required as auth[3] seems to be allocated memory which is unique in a
> system,
You lost me. Where do you see auth[3] getting encoded, and what do you
mean by saying it's "allocated memory which is unique in a system"?
There are a lot of different filehandle encoding options. I lose track
of them myself....
> just that we don't use it for locating the file currently and I was
> proposing
> that we do. Please correct me if this is wrong, or whether a better way is
> possible.
>
> > This patch doesn't change anything--it only adds new lines. If you're
> > trying to make minimal changes, that's admirable, but break up the
> > patches in such a way that each patch shows that minimal change.
>
> To make each revision compile cleanly, I had to add the infrastructure and
> then delete the old one. So it looks like new code. The entire function and
> data structures are re-written so it is difficult to break into individual
> components where each will compile clean.
Breaking up a complex change into logical individual steps, each of
which compile and run cleanly, is sometimes tricky. So it may be that
there *is* some way to do it in this case, but that you just haven't
found it yet.
That said, it could be, as you say, that this requires just replacing
everything. In that case, please just start over from scratch and don't
feel bound to keep stuff from the old implementation that you don't
understand.
--b.
> > By "noisy" I just mean that they look like there's a lot of randomness.
> > So, yes, I'm fine with a subset, but I'm curious what the variance is on
> > repeated runs of each test.
>
> Sure, I will try to do this over the weekend and submit.
>
> thanks,
>
> - KK
>