Linux-NFS Archive on lore.kernel.org
 help / color / Atom feed
From: Olga Kornievskaia <olga.kornievskaia@gmail.com>
To: "J. Bruce Fields" <bfields@redhat.com>
Cc: linux-nfs <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v5 5/9] NFSD add COPY_NOTIFY operation
Date: Thu, 29 Aug 2019 15:23:43 -0400
Message-ID: <CAN-5tyHOr+0WVthzu6G9qOwaVG0R+yUpuh2iyaTpBkw_O5XGAA@mail.gmail.com> (raw)
In-Reply-To: <CAN-5tyEX=4pfntZudpfkN9Pr9OpZfpvKj+NyT-bBD9YJ5fsg7Q@mail.gmail.com>

On Wed, Aug 14, 2019 at 11:05 AM Olga Kornievskaia
<olga.kornievskaia@gmail.com> wrote:
>
> On Tue, Aug 13, 2019 at 1:57 PM Olga Kornievskaia
> <olga.kornievskaia@gmail.com> wrote:
> >
> > On Mon, Aug 12, 2019 at 4:00 PM J. Bruce Fields <bfields@redhat.com> wrote:
> > >
> > > On Mon, Aug 12, 2019 at 03:16:47PM -0400, Olga Kornievskaia wrote:
> > > > On Mon, Aug 12, 2019 at 12:19 PM Olga Kornievskaia
> > > > <olga.kornievskaia@gmail.com> wrote:
> > > > > While this passes my testing, in theory this allows for the race that
> > > > > we get the copy notify size but then offload_cancel arrive and change
> > > > > the value. Then refcount_sub_and test_check would have an incorrect
> > > > > value (can subtract larger than an actual reference count). I have no
> > > > > solution for that as there is no refcount_sub_and_lock() that will
> > > > > allow to decrement by a multiple under a lock. Thoughts?
> > > >
> > > > I tried not to use the client's cl_lock but instead use a specific
> > > > lock to protect the copy notifies stateid on the stateid list. But
> > > > since stateid's reference counter (sc_count) is protected by it, I
> > > > think by getting rid of the special lock and using cl_lock will solve
> > > > the problem of coordinating access between the sc_count and the
> > > > copy_notify stateid list. Are the any problems with using such a big
> > > > lock?
> > >
> > > Probably not.  But it can be confusing when a single lock is used for
> > > several different things.  A comment explaining why you need it might
> > > help.
> >
> > While holding the client's cl_lock to manipulate the list of copy
> > notify stateids solves the refcount problem. It generates a different
> > problem for the laundromat thread. There, client list is traversed
> > already holding the cl_lock, so I can't call routines to free
> > copy_notify stateid because in turn it calls nfs4_put_stid() which
> > wants to take the cl_lock. Putting the copy_notify stateid on the
> > reaplist and then I lose a pointer to the client structure that I need
> > to take the lock. Then it seems the nfs4_cpntf_state structure would
> > need to keep a pointer to the client structure but then I get a
> > problem of making sure the nfs4_client structure isn't going away and
> > because it even a bigger mess.
> >
> > I think I need to remove the code in the laundromat that looks for the
> > not referenced copy_notifies stateid and just rely on cleaning on the
> > removal of the stateid (basically what I originally had). Or I need to
> > rely on the client to always send FREE_STATEID. I don't see other
> > options, do you?
>
> Ignore this Bruce. Trond gave me a good idea and gets me unstuck.

Hi Bruce,

I'm stuck again. The idea that Trond gave me is to instead of storing
the pointer to the stateid, (copy) store the stateid_t structure
itself and then use it to look it up the appropriate nfs4_stid.

The problem with that is when nfsd4_lookup_stateid() is called it
takes is a compound state (cstate) which has a client pointer and
during the lookup it's verified that the client looking up the stateid
is the same that generate the stateid which is not the case with copy
offload.

I tried also saving a cl_clientid and using that to lookup the
nfs4_client that's needed for the stateid lookup but I'm not sure
that's possible. lookup_clientid() calls find_client_in_id_table() and
always passes "false" for sessions args. Original client has minor
version 2 and then the check if (clp->minor_versions != sessions)
fails. I don't understand what this logic is suppose to check.

Should I be writing special version of the lookup_clientid that
ignores that check (when called in the path of the copy_notify
verification)? Or any other ideas of how to get passed this would be
appreciated it.

Thank you.


>
> >
> > >
> > > --b.

  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-08 20:18 [PATCH v5 0/9] server-side support for "inter" SSC copy Olga Kornievskaia
2019-08-08 20:18 ` [PATCH v5 1/9] NFSD fill-in netloc4 structure Olga Kornievskaia
2019-08-11  5:48   ` kbuild test robot
2019-08-12 16:12     ` Olga Kornievskaia
2019-08-12 19:58       ` J. Bruce Fields
2019-08-08 20:18 ` [PATCH v5 2/9] NFSD add ca_source_server<> to COPY Olga Kornievskaia
2019-08-11  5:59   ` kbuild test robot
2019-08-11  7:00   ` kbuild test robot
2019-08-08 20:18 ` [PATCH v5 3/9] NFSD return nfs4_stid in nfs4_preprocess_stateid_op Olga Kornievskaia
2019-08-08 20:18 ` [PATCH v5 4/9] NFSD COPY_NOTIFY xdr Olga Kornievskaia
2019-08-11  6:10   ` kbuild test robot
2019-08-08 20:18 ` [PATCH v5 5/9] NFSD add COPY_NOTIFY operation Olga Kornievskaia
2019-08-11  6:17   ` kbuild test robot
2019-08-12 16:19   ` Olga Kornievskaia
2019-08-12 19:16     ` Olga Kornievskaia
2019-08-12 20:00       ` J. Bruce Fields
2019-08-12 20:00         ` J. Bruce Fields
2019-08-13 17:57         ` Olga Kornievskaia
2019-08-14 15:05           ` Olga Kornievskaia
2019-08-29 19:23             ` Olga Kornievskaia [this message]
2019-08-30 17:56               ` bfields
2019-08-08 20:18 ` [PATCH v5 6/9] NFSD check stateids against copy stateids Olga Kornievskaia
2019-08-08 20:18 ` [PATCH v5 7/9] NFSD generalize nfsd4_compound_state flag names Olga Kornievskaia
2019-08-08 20:18 ` [PATCH v5 8/9] NFSD: allow inter server COPY to have a STALE source server fh Olga Kornievskaia
2019-08-08 20:18 ` [PATCH v5 9/9] NFSD add nfs4 inter ssc to nfsd4_copy Olga Kornievskaia
2019-08-11  6:24   ` kbuild test robot
2019-09-04 20:50 ` [PATCH v5 0/9] server-side support for "inter" SSC copy bfields
2019-09-05  0:05   ` Olga Kornievskaia
2019-09-05  0:13     ` Rick Macklem
2019-09-06 14:23       ` Olga Kornievskaia
2019-09-06 15:32         ` Rick Macklem

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAN-5tyHOr+0WVthzu6G9qOwaVG0R+yUpuh2iyaTpBkw_O5XGAA@mail.gmail.com \
    --to=olga.kornievskaia@gmail.com \
    --cc=bfields@redhat.com \
    --cc=linux-nfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-NFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nfs/0 linux-nfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nfs linux-nfs/ https://lore.kernel.org/linux-nfs \
		linux-nfs@vger.kernel.org linux-nfs@archiver.kernel.org
	public-inbox-index linux-nfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-nfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox