Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <eaf758e8-077d-f7fe-1e02-cfaa49830d97@oracle.com>
Date:   Fri, 15 Apr 2022 07:56:06 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.8.0
Subject: Re: [PATCH RFC v19 06/11] NFSD: Update find_clp_in_name_tree() to
 handle courtesy client
Content-Language: en-US
From:   dai.ngo@oracle.com
To:     Bruce Fields <bfields@fieldses.org>
Cc:     Chuck Lever III <chuck.lever@oracle.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
References: <1648742529-28551-1-git-send-email-dai.ngo@oracle.com>
 <1648742529-28551-7-git-send-email-dai.ngo@oracle.com>
 <20220401152109.GB18534@fieldses.org>
 <52CA1DBC-A0E2-4C1C-96DF-3E6114CDDFFD@oracle.com>
 <8dc762fc-dac8-b323-d0bc-4dbeada8c279@oracle.com>
 <20220413125550.GA29176@fieldses.org>
 <fae06919-13de-9ebb-8259-108f6e18c801@oracle.com>
In-Reply-To: <fae06919-13de-9ebb-8259-108f6e18c801@oracle.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?anlHVEZIdmVaREg2RTFCd3FCa2p1Y1lhOUhaTklkSXp2YjVUbjI0dzBWNWtm?=
 =?utf-8?B?am8rKzVtOFZmMEdFa1pBV0hRY2s4Yms1dWw3RHh1bmc4U2orVlFmcDRIZFpW?=
 =?utf-8?B?RGhOMjNxbm5maiswTmVrWUVQdlVyZDJDNi9FUTVUZkVJV2kyZnRBMGU1ZkUz?=
 =?utf-8?B?S05GdGtKRVVhYWE0bTcvSXBkZHpIU09KR0Nab2pxSXVhRW85eUczUXRqajVx?=
 =?utf-8?B?YUx3ZWVWZTBTaFpzMU84c1BuZDNRVzQyTWRBUFgvSXllMUdxZ2x2Z2dPeGNS?=
 =?utf-8?B?MUJBYWxwcEpyNTNXcVBhMlFhZHFobEVRWFlBOEYzaTMzTXFZSlVJYjdNUU9M?=
 =?utf-8?B?V3FGL1Q1dXVhYUFPQW5zR2kzZHE5RFpSRWdvY2sraHlFSFVFMy9DQXlXUUQ3?=
 =?utf-8?B?VUlFN1JUY05QNzdWd2NTTmNDVUxYRHlock9WZG93a2RZYm5mcWNGbURQL2hl?=
 =?utf-8?B?TXBLRUVxL0p4T0daYUk4SWlOWEt3RUFYUUxPTytodnFEcSszL2FqekZvOHlO?=
 =?utf-8?B?dCtGSHVjM0N6TUppRUlsTDhuVVlZMUd1cjlEKzJxeVFVYUozTUJCczQzcldx?=
 =?utf-8?B?T05qaUNXcStCREpqNzhnS2FxVThldDBoblpld1pjUERHbVhCWDd4RkdBNEtH?=
 =?utf-8?B?RHpzbUFXVHJxcWhSNlFJdE5vV0JpNGFNZkdSK2lCMFRlTVJqSG5WT3NTUnRW?=
 =?utf-8?B?aVNzcC90RThnay9STjA2VEswN0dMQnBhYTZsYTRNNFFCTDBOSmswWTV6citS?=
 =?utf-8?B?bkNDM3B1bjVuWmZRWHBZWlBhSkxNMDVNNm56V1BtNmRBb3JMbTBUcUVuRUxu?=
 =?utf-8?B?TDloalh5c01vR3B6VmtQa3d1bU8wQmtSM3BXU1E1Q0p3VENuUGZTR3NwNXhV?=
 =?utf-8?B?LzBUVW55eTBhNzBSWDUrRzYwS004UDJOT2VRUFE1b2FvcE1PSldiQSs1Z0U4?=
 =?utf-8?B?UzF1N0F0WVRGN2Z3bDY3TTJiS3d6am0yUkhLS0NucVFTQnJKbVJVeXE1M3l6?=
 =?utf-8?B?Q0FBLy96ZFdKbHBtZkZsUGZaalNSRnlvZTVoRkFncC9IZG5uTUpGckpMT2xH?=
 =?utf-8?B?V1BMaTYwSTVSRElTQ1hsVXpFK3JmRDRFMTR4Y0JQalZMNWJFMGd5akpVeEVC?=
 =?utf-8?B?NWFselFaSWZESVc1Z1lzd2dnUFBZTGxkSmk3QWJuckwzMWl3Z1ArYjFleTdV?=
 =?utf-8?B?MWtvMWVMd3ZBSkEzWXd2c0xmbXNTSGdscXJncXlYL3JCa0ZTY054aVJuby8w?=
 =?utf-8?B?TU9CSFRSTzE0Wk1PQzJyNmlHTjhuYk9Cczl1bGJQV1JoZDlZRTdJaEtZVGJw?=
 =?utf-8?B?R1J0SXo2ZnVUWWs5N3ZONUw3YTRKdnJuZnNod05BUkpjOVovZ0d1aGwrekFp?=
 =?utf-8?B?VWpGN1NIUGMwNDRVVVBjNTZreko0bkI4REJ2dkUyMEErVFZHR2tRRDIxcDAw?=
 =?utf-8?B?ckR6K2J3SmhLeWZvT2VJTmhNQTRUK2lYMFJrT0RMckFxTC92UUFLQ28rZVdU?=
 =?utf-8?B?Nk1FV2s4a3FqdC9NYytWekdXUEtZaVVpbzZYMjJCMGYxbjgydkJYUXdETjNR?=
 =?utf-8?B?eHBQVWZLbUdUaTlHNDFEM3FJV3lGWW5NRjZWU2pJaittbUd6WFhpcERYc09l?=
 =?utf-8?B?ZEhXWkpvbENDcmdyL1Y5czlHd3JDeE5zaTJZZkhBVDcraitveWtRdi9GRU5C?=
 =?utf-8?B?VUdKR1Z3R0lqQjlQaUZZeTIzd0k1N2w5N3E4V1k1OTNiazBLalp1VUZnQnlG?=
 =?utf-8?B?Q0RSUGZHVmVBaGJEWmNvZnJhU1J5QTU2RmlyZWRrQ0lHa0tPZk5vSGRrcVJQ?=
 =?utf-8?B?ajM0WWRYMjZ5czJNd2h4UWhEREthMHpwbkpOQ2RLOFVEcUFBOFlQb2Y0WGV6?=
 =?utf-8?B?eXpuZjJsN3ZsdVhoK1pjWUVXZkd1VjZjTVBDRFZuTGJBQnlPak1aT2ovb0tQ?=
 =?utf-8?B?RzlNeldFSDQxWmhaSTJQVWlyR2IzYUpKMy9Md1pKSGl5QVdoTVRMY0I4MzQ0?=
 =?utf-8?B?VjErT2Fsbmpma0ROTE5icnJUL1hSeUgwM1dpTDJWOXZBQmhKelhlMTUrNFp4?=
 =?utf-8?B?R2hzOHRRZ0JLaDA0WE82TWRsWVZkQlBRVlNvS1c2Ym1sS1ZBbFlSRDJ0dWQz?=
 =?utf-8?B?dkNPdTZVMmNTZ1N6OW5hR1cvc0RaRnd0K0VwMFByL0NHNjR3RFZXcXVzR2Jn?=
 =?utf-8?B?Q0g0SFBoU0FJd2VkekNqd2UrMXUvbW43Qk9OaGNwSXVwNXhmZDhaTGIvRlY3?=
 =?utf-8?B?RENiUm5sMXBRUXRTaTFWMyt1QktJVktmaU5pZVNHb2x5ZmM5ZTVndklsOWox?=
 =?utf-8?B?YXZBdUs0ZTdoVmFnSXcwZnc5cjc2YWFuODBrVUREbXJOanZyM2FZQT09?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a4af548f-0e53-4547-f37e-08da1ef0126b
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Apr 2022 14:56:07.7176
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: YEGhIfphyth4qHNA55loEx9d5pSrtfFMof+rplVe6tubiZJNTextGZud80AKsh5SIrTrpa7NTxzbe/KiPy8p4Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR10MB1553
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-04-15_01:2022-04-14,2022-04-15 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 malwarescore=0
 mlxscore=0 phishscore=0 suspectscore=0 spamscore=0 adultscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2204150088
X-Proofpoint-GUID: lkMwQIsbtb1xTPy1I6AYavnhVAIn48rZ
X-Proofpoint-ORIG-GUID: lkMwQIsbtb1xTPy1I6AYavnhVAIn48rZ
X-Spam-Status: No, score=-6.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


On 4/15/22 7:47 AM, dai.ngo@oracle.com wrote:
>
> On 4/13/22 5:55 AM, Bruce Fields wrote:
>> On Fri, Apr 01, 2022 at 12:11:34PM -0700, dai.ngo@oracle.com wrote:
>>> On 4/1/22 8:57 AM, Chuck Lever III wrote:
>>>>> (And to be honest I'd still prefer the original approach where we 
>>>>> expire
>>>>> clients from the posix locking code and then retry.  It handles an
>>>>> additional case (the one where reboot happens after a long network
>>>>> partition), and I don't think it requires adding these new client
>>>>> states....)
>>>> The locking of the earlier approach was unworkable.
>>>>
>>>> But, I'm happy to consider that again if you can come up with a way
>>>> of handling it properly and simply.
>>> I will wait for feedback from Bruce before sending v20 with the
>>> above change.
>> OK, I'd like to tweak the design in that direction.
>>
>> I'd like to handle the case where the network goes down for a while, and
>> the server gets power-cycled before the network comes back up. I think
>> that could easily happen.  There's no reason clients couldn't reclaim
>> all their state in that case.  We should let them.
>>
>> To handle that case, we have to delay removing the client's stable
>> storage record until there's a lock conflict.  That means code that
>> checks for conflicts must be able to sleep.
>>
>> In each case (opens, locks, delegations), conflicts are first detected
>> while holding a spinlock.  So we need to unlock before waiting, and then
>> retry if necessary.
>>
>> We decided instead to remove the stable-storage record when first
>> converting a client to a courtesy client--then we can handle a conflict
>> by just setting a flag on the client that indicates it should no longer
>> be used, no need to drop any locks.
>>
>> That leaves the client in a state where it's still on a bunch of global
>> data structures, but has to be treated as if it no longer exists.  That
>> turns out to require more special handling than expected. You've shown
>> admirable persistance in handling those cases, but I'm still not
>> completely convinced this is correct.
>>
>> We could avoid that complication, and also solve the
>> server-reboot-during-network-partition problem, if we went back to the
>> first plan and allowed ourselves to sleep at the time we detect a
>> conflict.  I don't think it's that complicated.
>>
>> We end up using a lot of the same logic regardless, so don't throw away
>> the existing patches.
>>
>> My basic plan is:
>>
>> Keep the client state, but with only three values: ACTIVE, COURTESY, and
>> EXPIRABLE.
>>
>> ACTIVE is the initial state, which we return to whenever we renew.  The
>> laundromat sets COURTESY whenever a client isn't renewed for a lease
>> period.  When we run into a conflict with a lock held by a client, we
>> call
>>
>>    static bool try_to_expire_client(struct nfs4_client *clp)
>>    {
>>     return COURTESY == cmpxchg(clp->cl_state, COURTESY, EXPIRABLE);
>>    }
>>
>> If it returns true, that tells us the client was a courtesy client.  We
>> then call queue_work(laundry_wq, &nn->laundromat_work) to tell the
>> laundromat to actually expire the client.  Then if needed we can drop
>> locks, wait for the laundromat to do the work with
>> flush_workqueue(laundry_wq), and retry.
>>
>> All the EXPIRABLE state does is tell the laundromat to expire this
>> client.  It does *not* prevent the client from being renewed and
>> acquiring new locks--if that happens before the laundromat gets to the
>> client, that's fine, we let it return to ACTIVE state and if someone
>> retries the conflicing lock they'll just get a denial.
>>
>> Here's a suggested a rough patch ordering.  If you want to go above and
>> beyond, I also suggest some tests that should pass after each step:
>>
>>
>> PATCH 1
>> -------
>>
>> Implement courtesy behavior *only* for clients that have
>> delegations, but no actual opens or locks:
>
> we can do a close(2) so that the only remaining state is the
> delegation state. However this causes problem for doing exactly
> what you want in patch 1.
>
>>
>> Define new cl_state field with values ACTIVE, COURTESY, and EXPIRABLE.
>> Set to ACTIVE on renewal.  Modify the laundromat so that instead of
>> expiring any client that's too old, it first checks if a client has
>> state consisting only of unconflicted delegations, and, if so, it sets
>> COURTESY.
>>
>> Define try_to_expire_client as above.  In nfsd_break_deleg_cb, call
>> try_to_expire_client and queue_work.  (But also continue scheduling the
>> recall as we do in the current code, there's no harm to that.)
>>
>> Modify the laundromat to try to expire old clients with EXPIRED set.
>>
>> TESTS:
>>     - Establish a client, open a file, get a delegation, close the
>>       file, wait 2 lease periods, verify that you can still use the
>>       delegation.
>
> From user space, how do we force the client to use the delegation
> state to read the file *after* doing a close(2)?

I guess we can write new pynfs test to do this, but I'd like to avoid
it if possible.

-Dai
  

>
> In my testing, once the read delegation is granted the Linux client
> uses the delegation state to read the file. So the test can do open(2),
> read some (more(1)), wait for 2 lease periods and read again and
> make sure it works as expected.
>
> Can we leave the open state remain valid for this patch but do not
> support share reservation conflict yet until Patch 2?
>
> -Dai
>
>>     - Establish a client, open a file, get a delegation, close the
>>       file, wait 2 lease periods, establish a second client, request
>>       a conflicting open, verify that the open succeeds and that the
>>       first client is no longer able to use its delegation.
>>
>>
>> PATCH 2
>> -------
>>
>> Extend courtesy client behavior to clients that have opens or
>> delegations, but no locks:
>>
>> Modify the laundromat to set COURTESY on old clients with state
>> consisting only of opens or unconflicted delegations.
>>
>> Add in nfs4_resolve_deny_conflicts_locked and friends as in your patch
>> "Update nfs4_get_vfs_file()...", but in the case of a conflict, call
>> try_to_expire_client and queue_work(), then modify e.g.
>> nfs4_get_vfs_file to flush_workqueue() and then retry after unlocking
>> fi_lock.
>>
>> TESTS:
>>     - establish a client, open a file, wait 2 lease periods, verify
>>       that you can still use the open stateid.
>>     - establish a client, open a file, wait 2 lease periods,
>>       establish a second client, request an open with a share mode
>>       conflicting with the first open, verify that the open succeeds
>>       and that first client is no longer able to use its open.
>>
>> PATCH 3
>> -------
>>
>> Minor tweak to prevent the laundromat from being freed out from
>> under a thread processing a conflicting lock:
>>
>> Create and destroy the laundromat workqueue in init_nfsd/exit_nfsd
>> instead of where it's done currently.
>>
>> (That makes the laundromat's lifetime longer than strictly necessary.
>> We could do better with a little more work; I think this is OK for now.)
>>
>> TESTS:
>>     - just rerun any regression tests; this patch shouldn't change
>>       behavior.
>>
>> PATCH 4
>> -------
>>
>> Extend courtesy client behavior to any client with state, including
>> locks:
>>
>> Modify the laundromat to set COURTESY on any old client with state.
>>
>> Add two new lock manager callbacks:
>>
>>     void * (*lm_lock_expirable)(struct file_lock *);
>>     bool (*lm_expire_lock)(void *);
>>
>> If lm_lock_expirable() is called and returns non-NULL, posix_lock_inode
>> should drop flc_lock, call lm_expire_lock() with the value returned from
>> lm_lock_expirable, and then restart the loop over flc_posix from the
>> beginning.
>>
>> For now, nfsd's lm_lock_expirable will basically just be
>>
>>     if (try_to_expire_client()) {
>>         queue_work()
>>         return get_net();
>>     }
>>     return NULL;
>>
>> and lm_expire_lock will:
>>
>>     flush_workqueue()
>>     put_net()
>>
>> One more subtlety: the moment we drop the flc_lock, it's possible
>> another task could race in and free it.  Worse, the nfsd module could be
>> removed entirely--so nfsd's lm_expire_lock code could disappear out from
>> under us.  To prevent this, I think we need to add a struct module
>> *owner field to struct lock_manager_operations, and use it like:
>>
>>     owner = fl->fl_lmops->owner;
>>     __get_module(owner);
>>     expire_lock = fl->fl_lmops->lm_expire_lock;
>>     spin_unlock(&ctx->flc_lock);
>>     expire_lock(...);
>>     module_put(owner);
>>
>> Maybe there's some simpler way, but I don't see it.
>>
>> TESTS:
>>     - retest courtesy client behavior using file locks this time.
>>
>> -- 
>>
>> That's the basic idea.  I think it should work--though I may have
>> overlooked something.
>>
>> This has us flush the laundromat workqueue while holding mutexes in a
>> couple cases.  We could avoid that with a little more work, I think.
>> But those mutexes should only be associated with the client requesting a
>> new open/lock, and such a client shouldn't be touched by the laundromat,
>> so I think we're OK.
>>
>> It'd also be helpful to update the info file with courtesy client
>> information, as you do in your current patches.
>>
>> Does this make sense?
>>
>> --b.