Re: [PATCH v1 12/13] NFSD: allow inter server COPY to have a STALE source server fh

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Nov 08, 2018 at 02:25:02PM -0500, J. Bruce Fields wrote:
> On Thu, Nov 08, 2018 at 01:51:58PM -0500, Olga Kornievskaia wrote:
> > On Wed, Nov 7, 2018 at 1:57 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> > >
> > > On Fri, Oct 19, 2018 at 11:29:04AM -0400, Olga Kornievskaia wrote:
> > > > From: Olga Kornievskaia <kolga@netapp.com>
> > > >
> > > > The inter server to server COPY source server filehandle
> > > > is a foreign filehandle as the COPY is sent to the destination
> > > > server.
> > >
> > > Compounds can do a lot of different strange things, and I'm not
> > > convinced this code handles every case correctly.  Examples: I think
> > > that
> > >
> > >         PUTFH
> > >         TEST_STATEID
> > >         SAVEFH
> > >         COPY
> > >
> > > will incorrectly return nfserr_stale if the PUTHF gets a foreign
> > > filehandle, even though that filehandle is only used as the source of
> > > the COPY.  And:
> > >
> > >         PUTFH
> > >         SAVEFH
> > >         RENAME
> > >         COPY
> > >
> > > will pass an unverified source filehandle to rename.
> > >
> > > I can think of a couple ways to get this right for certain:
> > >
> > >         - delay all filehandle verification till the time the filehandle
> > >           isused.  That would make checking this simple, but it would
> > >           change our behavior so, for example PUTFH+READ with a bad
> > >           filehandle will return the error on the READ where it used to
> > >           return it on the PUTFH.  I don't know if that's a problem.
> > >
> > >         - somewhere at the start of nfsd4_proc_compound, do one pass
> > >           through the compound checking where the filehandles will be
> > >           used and marking those ops that can skip checking.  E.g.:
> > >
> > >                 nfsd4_op *current, *saved
> > >
> > >                 foreach op in compound:
> > >                         - if op is putfh:
> > >                                 current := op
> > >                         - if op is savefh:
> > >                                 saved := current
> > >                         - if op is restorefh:
> > >                                 current := saved
> > >                         - etc.
> > >                         - if op is copy:
> > >                                 mark_no_verify(saved)
> > >
> > >           Or something like that.
> > 
> > Do you have a preference over the 2 proposed methods? I'm not sure if
> > there is anything wrong with returning ERR_STALE  on READ instead of
> > the PUTFH but for historical reasons it seems wrong to change it. Thus
> > I'd say doing it the 2nd way is better. But then 2nd approach adds an
> > overhead of going thru operations twice for any compound. Is that
> > acceptable?
> 
> I think so.  Most compounds are pretty short and I don't think it'll be
> a big deal.

So, yes, could you try the second approach?

I've been working on the 1st approach--I have a patch here and I may
experiment with it some more.  But I'm starting to think that the
separate scan will work better.

--b.