DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BD64E204FD
Date:   Thu, 8 Nov 2018 14:25:02 -0500
From:   "J. Bruce Fields" <bfields@fieldses.org>
To:     Olga Kornievskaia <olga.kornievskaia@gmail.com>
Cc:     "J. Bruce Fields" <bfields@redhat.com>,
        linux-nfs <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v1 12/13] NFSD: allow inter server COPY to have a STALE
 source server fh
Message-ID: <20181108192502.GB6090@fieldses.org>
References: <20181019152905.32418-1-olga.kornievskaia@gmail.com>
 <20181019152905.32418-13-olga.kornievskaia@gmail.com>
 <20181107185753.GB19588@fieldses.org>
 <CAN-5tyH7XWsnf8VU-sMDTcxFdvk-Q7=ST5hHz=uxSk01xDk7ug@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAN-5tyH7XWsnf8VU-sMDTcxFdvk-Q7=ST5hHz=uxSk01xDk7ug@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

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.

> I have to ask: for simplicify can't we just support COPY compound if
> and only if it's in a specific order and then only allow it?

We could probably narrow the possibilities down to a few, but I'm a
little afraid of overlooking some possible creative client behavior.

I don't think it's that hard to follow the spec here, and it may be
simpler than verifying an argument about which cases matter.

--b.