DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 85A8A204FD
MIME-Version: 1.0
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>
 <20181108192502.GB6090@fieldses.org> <20181108192759.GC6090@fieldses.org>
In-Reply-To: <20181108192759.GC6090@fieldses.org>
From:   Olga Kornievskaia <olga.kornievskaia@gmail.com>
Date:   Thu, 8 Nov 2018 14:31:37 -0500
Message-ID: <CAN-5tyG9+-O+htFXiZ-PSj16QkVPQmWsDNNXbFmusK1Yd4-RwQ@mail.gmail.com>
Subject: Re: [PATCH v1 12/13] NFSD: allow inter server COPY to have a STALE
 source server fh
To:     "J. Bruce Fields" <bfields@fieldses.org>
Cc:     "J. Bruce Fields" <bfields@redhat.com>,
        linux-nfs <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Thu, Nov 8, 2018 at 2:28 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> 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.