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: <9ac39863-e4be-8783-cd00-21d79d9baa56@oracle.com>
Date:   Wed, 24 May 2023 13:27:47 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.11.0
Subject: Re: [PATCH v5 3/3] locks: allow support for write delegation
Content-Language: en-US
To:     Jeff Layton <jlayton@kernel.org>,
        Chuck Lever III <chuck.lever@oracle.com>
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
References: <1684799560-31663-1-git-send-email-dai.ngo@oracle.com>
 <1684799560-31663-4-git-send-email-dai.ngo@oracle.com>
 <32e880c5f66ce8a6f343c01416fcc8b791cc1302.camel@kernel.org>
 <D8739068-BCAD-4E47-A2E2-1467F9DC32ED@oracle.com>
 <bc960c7251781f912d2d0d4271702d15f19fb34a.camel@kernel.org>
 <CDB5013B-A8D2-4035-9210-B0854B1EE729@oracle.com>
 <b4c4d608-80aa-7f3d-7a83-2e7b24918b02@oracle.com>
 <dc0c053d734dc5b45475cde015d1cebb78922336.camel@kernel.org>
From:   dai.ngo@oracle.com
In-Reply-To: <dc0c053d734dc5b45475cde015d1cebb78922336.camel@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dGgySitXbXdKVUJzenNjb2JVSTh2M2VIQm5Rcko4SUJGTWFoWk9KRXB2VkZ6?=
 =?utf-8?B?Q1M0elhpWmJoMmQ0UElTNVgvS0JLRmxLSFNuZ0IzYzhwMVQ1K1JMOXdaeFZ6?=
 =?utf-8?B?Vkwvd0xYNW5hTUZOSkovcWpRRklmc0M3cWJkVDJiZVMzQVUyYVdFNHhrcFJx?=
 =?utf-8?B?b2pveTFCYmFLZ21LcGtTTDFwMkt4cFNrdmNkWmR3Wjl2UUM0aWRwZ2JlaHFL?=
 =?utf-8?B?MDN0WXZXeXZkWjNZWVk5WFZtRkc3UWZoeVFZaWJkZUFXRm8rMDVXQWt3VmE0?=
 =?utf-8?B?Y1dNZDZzdUdRSnMxK2thWkpOTitsVUp1QmQ5SlBpaDA2MEJjRmQ1ZUQ0MllK?=
 =?utf-8?B?Tno2MjhvWGMxL3dVTWtOTWZwTlNZaU52di9hSUV3VUptZERtT2RGdFYzVmhv?=
 =?utf-8?B?TXhyM2l5RDdrdERIZzc5UnpzQXBZQmhBR0VFRkU4MnRMNzFPdVhzR2I0WnUz?=
 =?utf-8?B?MW1oa2R2VlplQ2NGMDJrcXEyRWU2emVLb2IzUjFXUkpMaTY4ZVNNV2hPVG5m?=
 =?utf-8?B?SFFDdUFQOFhxL2tOc2tCZHFBdVdTNlplaFJQU1JONlZPWVcyb2d5U3VSYmJP?=
 =?utf-8?B?eUpvVEF3elR6ZWdETFBFcWdFNllFckxHQXpYU0dHbllGUmhCdWd6UlorRzEy?=
 =?utf-8?B?cjNMMTAzbHpBL00xSkhpMklBZ09vdU5OeEJaUnBUaTJhbCtJNkVmcWt3N0dM?=
 =?utf-8?B?M3liWUlBbncwMGdUOVB6SFJxWk1ZNDNkeWM4M3ZCWGZlRURvVU9OK1JoeGFM?=
 =?utf-8?B?SVVPeEFlMGNjVDNFV3hUcWVtelVHcGdtZHJrV1hMRnVIak41REUrTy9tVEdk?=
 =?utf-8?B?NXRPUi9uczRPd29tSjh0K2NIOVQveDArdTBtRzY4enJ6azNlYkZxcW4zbDNt?=
 =?utf-8?B?UmRqMUxpQ3BRc2NQWnFmNFVGcW1DZnEySVVreDlNcThzdHFiT1FQY0FhSFNv?=
 =?utf-8?B?K2hOVzZZQzlxTUdhejhnM1l0MjhpTjByNUJ1b3hFQkEyM0c2aHVKTlVHcGhy?=
 =?utf-8?B?OVc5YTZYbzN3WHc5d0tkazZPbi9zYUJpN2c2ay9FQmFpZ0VwV0djWWJPaDhk?=
 =?utf-8?B?YVhjZWtTWnI3VWdGeUg2elBNK1BQbHdSNHRTME1RSXhvY2NUMEprdlVWb29R?=
 =?utf-8?B?YVJMMUJtTTlzdTNGRENpc0toNWJhaURXblpVbHVSeFZLeW1idXRUYllUTG9V?=
 =?utf-8?B?MWYrc2VXcFE2WkU5MU0wZlNyRnRzZ1F0Z2lwbVFUajRxUmZkZTdMUWNZNTNm?=
 =?utf-8?B?K21YQVdkODBkMUl5YU8wSXpTZldMZ2ZRR09GYzllbm9hMkNjYkFRakU4NFBQ?=
 =?utf-8?B?bmlUS0lwWlBNa21raWpEd3VXZ0xFNVhWa3h0OEE3V1VFNmxqU0dUU0YzZDRz?=
 =?utf-8?B?bExyTUZ5RjhEU0wrdkh1TzYxckxja3VWZEl3OG51MUh1ajdEVnhOL2VrcGJi?=
 =?utf-8?B?cFNLUGRac1BhZkJGRGhPWHpvVTRkbklaODhrWWt1SXJxL0o1NVVGRjl0QUFq?=
 =?utf-8?B?WUZPYTNsNE5ZV2tMNXZlNHlweW1lSHZVb05EWllHeDl0UUR5azduN09rb2FV?=
 =?utf-8?B?Q3F1dFhPR055aWJhQUpnY3hNZHdHd0xOU1dteDUvM1puM1drbjhIM0FyMTdq?=
 =?utf-8?B?dzJoOWNrTnoxamZSdWpHNDRMZlVldm9Yc0I1VytZVlJtZFVaejcwMTQwalJV?=
 =?utf-8?B?bk1kRzBJNlpUQmNGOFRIY1I4WDBvMThWN0N3a25rM09VV0tCZE1kL2pRUHNa?=
 =?utf-8?B?cXZia24zcGZpeCtIK3N6Y1FlN0tlUkh6SVp0ellXZkJtaFY3RmhNYXpBZm1C?=
 =?utf-8?B?dVZwcEw4ZUVqSmxudFF4Y3hYZldhT3hNcFVzaEJLR29YckR5ZXhNL1NJL29n?=
 =?utf-8?B?ZmdiS25pa0xFeEh3cFVqWmQxaGRvOGt5b0RuV1kwYlNaQ1hCTFB2U2ZKRnJ1?=
 =?utf-8?B?MGZnK0p5MmxjZU11WUVLQkRlZ1dpTDdFVTRrMVZMY3dQNUpSMmdIQ3FTNGlS?=
 =?utf-8?B?YkhHVmZQeHhMcWFCZ0doYW5wRk5ZRW9oaHFIVHdTQXU3QmQ3NXVuQkNSdmFo?=
 =?utf-8?B?ZzlEeDlxWHl6WnEyTkVoZDk5QThGZkJnMjZtc29xUCttWEM4aGlwSFgzNU9u?=
 =?utf-8?Q?KCkYyhgYd1glGHe9df76snC/S?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: P71kYd52EW1Nhv4zDJfBbKit8nvKecZ2Sa0f5PvhxBYI/cs6Evkaa/uYaW4Bbvl9V7dW53fIObwS7ZGTkM+svpdIV5tF3d9O9BM4Ydur/MV6jOMdsO0a3RxzJE8i09QV9kEA7xOs7fKozHRy6fShkkqIOELk4sGbDJv9Xn0eCuXvdBqyyk5vjP2Oo4Jk6mAticLoffYbpTPiOzyHTLaBq5teu90Rk1Hni1G0Bw7zldX1RXKEz48FIRL6mxAzcPdsmuEyDLD03i8wf1g631iuBvYVIyxCX6eJIcAiDmOcY18vD3kUpjR+Xfkq8fozi3J1RMRDMB8CfHRyB4VqS3bOgWN1LTqB+jFhy8capIWQmLkKM6w/H+05l/oZ/XoyKxCJcyCWVFatukwhVg7W2xOjykGPy+WGsoxRdgol0iHsuO49UeiDTVFm/P15RPbewsD3tX/Lgic9IAy7woSFIbtuxKcpVL0W6m08mtTn75NsfThtlUwIlQYoJM9KTnF+YP+ieMBcolYxXlfVLNMmCnivEGtvUCdcljjoAn9Z10yCjrk2VNFhw4XVP9XSFdoKyJHZS8XNFYQ3kajW1aZYcsk8XOu67dDKWa0JABZBtQONNMRzFLvm5UPsawshxvNVEW2Jq3dNQ3qRGpj9k2b1GX8sdo6Au6Ow1tZ6EhmKhB1ALnOXdYRcnVrI3oIH/GkYTTQjj9X+NlZmy3voPBFoei2gPM1QEIHpm2TrXU/LN0X+m5dM+LqcYFV8zLtOIfxonYiFrYCp8Mk9CkRzbckLUaHRPtTsYJU9vJMNy+rk78CGZLM=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 79463db3-02eb-4d76-3c4c-08db5c9557f4
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 May 2023 20:27:50.1787
 (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: y77w7P6r5eTXacnOwX/K1l3pwg5eqqITFt3jkNtN5RythvyHiqk3j5YWyfrpPkULbiQ8HRnYVZHyddXhjrx3ug==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR10MB6810
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.176.26
 definitions=2023-05-24_15,2023-05-24_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 phishscore=0 mlxscore=0
 mlxlogscore=999 bulkscore=0 malwarescore=0 suspectscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2304280000
 definitions=main-2305240170
X-Proofpoint-ORIG-GUID: TUsyNnfsPrK-cDIyk5ZANPCqybTSYtXS
X-Proofpoint-GUID: TUsyNnfsPrK-cDIyk5ZANPCqybTSYtXS
X-Spam-Status: No, score=-2.9 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_H2,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 5/24/23 12:03 PM, Jeff Layton wrote:
> On Wed, 2023-05-24 at 11:05 -0700, dai.ngo@oracle.com wrote:
>> On 5/24/23 10:41 AM, Chuck Lever III wrote:
>>>> On May 24, 2023, at 12:55 PM, Jeff Layton <jlayton@kernel.org> wrote:
>>>>
>>>> On Wed, 2023-05-24 at 15:09 +0000, Chuck Lever III wrote:
>>>>>> On May 24, 2023, at 11:08 AM, Jeff Layton <jlayton@kernel.org> wrote:
>>>>>>
>>>>>> On Mon, 2023-05-22 at 16:52 -0700, Dai Ngo wrote:
>>>>>>> Remove the check for F_WRLCK in generic_add_lease to allow file_lock
>>>>>>> to be used for write delegation.
>>>>>>>
>>>>>>> First consumer is NFSD.
>>>>>>>
>>>>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>>>>> ---
>>>>>>> fs/locks.c | 7 -------
>>>>>>> 1 file changed, 7 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/locks.c b/fs/locks.c
>>>>>>> index df8b26a42524..08fb0b4fd4f8 100644
>>>>>>> --- a/fs/locks.c
>>>>>>> +++ b/fs/locks.c
>>>>>>> @@ -1729,13 +1729,6 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
>>>>>>> if (is_deleg && !inode_trylock(inode))
>>>>>>> return -EAGAIN;
>>>>>>>
>>>>>>> - if (is_deleg && arg == F_WRLCK) {
>>>>>>> - /* Write delegations are not currently supported: */
>>>>>>> - inode_unlock(inode);
>>>>>>> - WARN_ON_ONCE(1);
>>>>>>> - return -EINVAL;
>>>>>>> - }
>>>>>>> -
>>>>>>> percpu_down_read(&file_rwsem);
>>>>>>> spin_lock(&ctx->flc_lock);
>>>>>>> time_out_leases(inode, &dispose);
>>>>>> I'd probably move this back to the first patch in the series.
>>>>>>
>>>>>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>>>>> I asked him to move it to the end. Is it safe to take out this
>>>>> check before write delegation is actually implemented?
>>>>>
>>>> I think so, but it don't think it doesn't make much difference either
>>>> way. The only real downside of putting it at the end is that you might
>>>> have to contend with a WARN_ON_ONCE if you're bisecting.
>>> My main concern is in fact preventing problems during bisection.
>>> I can apply 3/3 and then 1/3, if you're good with that.
>> I'm good with that. You can apply 3/3 then 1/3 and drop 2/3 so I
>> don't have to send out v6.
>>
> I'm fine with that too, particularly if other vendors don't recall on a
> getattr currently.
>
> I wonder though, if we need some clarification in the spec on
> CB_GETATTR?
>
>      https://www.rfc-editor.org/rfc/rfc8881.html#section-10.4.3
>
> In that section, there is a big distinction about the client being able
> to tell that the data was modified from the point where the delegation
> was handed out.
>
> There is always a point in time where a client has buffered writes that
> haven't been flushed to the server yet, but that's true when it doesn't
> have a delegation too. Mostly the client tries to start some writeback
> fairly quickly so any lag how the in the change attr/size update is
> usually short lived.
>
> I don't think the Linux client materially changes its writeback behavior
> based on a write delegation, so I guess (as Olga pointed out) the main
> benefit from a write delegation is being able to do delegated opens for
> write. A getattr's results won't be changed by extra opens or closes, so
> yeah...I guess the utility of CB_GETATTR is really limited.
>
> I guess it _might_ be useful in the case where the server has handed out
> a write delegation, but hasn't gotten any writes. That would at least
> tell the client that something has changed, even if the deleg holder
> hasn't gotten around to writing anything back yet. The problem is that
> it's common for applications to open O_RDWR and only do reads.
>
> Maybe we ought to take this discussion to the IETF list? It seems like
> the spec mandates that you must recall the delegation if you don't
> implement CB_GETATTR, but I don't see much in way of harm in ignoring
> that.

Yes, I think we should, for clarification.
Jeff, would you mind to drive this discussion on IETF since you can
present the issue much clearer than I would.

Thanks,
-Dai