Received-SPF: pass (google.com: domain of linux-nfs+bounces-2004-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sat, 17 Feb 2024 12:05:42 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: Trond Myklebust <trondmy@hammerspace.com>
Cc: "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr()
Message-ID: <ZdDnZmQ5_rAUm6fl@manet.1015granger.net>
References: <20240216012451.22725-1-trondmy@kernel.org>
 <20240216012451.22725-2-trondmy@kernel.org>
 <Zc9kQ1Autf6xdcii@tissot.1015granger.net>
 <ac1166ca466c343f18df45094c0130947bd21f5c.camel@hammerspace.com>
 <CFBE3BDF-E347-4273-8C7F-A57E0D353457@oracle.com>
 <756FABF8-FA78-4D16-A4B8-B47C4745868E@oracle.com>
 <5c35277cd061e16a914b94e070ea6d95a75c1342.camel@hammerspace.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <5c35277cd061e16a914b94e070ea6d95a75c1342.camel@hammerspace.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?WmlTZ2pPdlNnWXFtbm45bnRrUms3YkVlalBzT2lwTHFFWmtDbWN0djRFelRI?=
 =?utf-8?B?N0FDUWZEQXMzUnFxRllvNkZyYndPbXhMcE95SzQ2V2NBRHd4RWpBSmpqeWpI?=
 =?utf-8?B?cUIvUExUL2JOeFJlLzREYUJqcVRlS2NVcnJJVmNoYmpnNW0xdHJjaWJoZmFY?=
 =?utf-8?B?bWZNYmUzcFA4NmxnaG5IcHpmTHBvc1lkS3pGQ2dsdWVPOXVGbE50WGx4Yitt?=
 =?utf-8?B?bEY1T0xEVmx0V1BucDAyWTNlMmJNS1M0YzBFZ1pmd2ladHdtbEs5T05IMVh1?=
 =?utf-8?B?ekQycWVRT1R1YUJJWjJGN01BWjJvZGRjWFFsWEplMVh2eTVBMnZqdXJaZVNN?=
 =?utf-8?B?V2p0T2drV1JPMG02TnF0cmY5bFNURkdiWnViaHEyT1VGUWFMQUMwSVhLaHlh?=
 =?utf-8?B?MzhTTHdFNEdMd1dQdnpqU0xFMVBlUE90cEM0eTFNUjlhL1NPNGx1WWllakNU?=
 =?utf-8?B?SDFBajR6YitDL0xqSFNBaU5CQVkwbEVlN0xqM0VseHlUMUFXK0VzaU9TQmxY?=
 =?utf-8?B?b1g3MlBjU3A0UVdhajRjeWxRRWVDNllFRFBtWnpGazllRDQ4NCsxb1NMUm9G?=
 =?utf-8?B?RjZiSzZFOTB3MFIvUmhsWWVMNkU5aVU2MXJYSjROTUtheUgzQ0NjUjg3WG1C?=
 =?utf-8?B?bE5HSTVZNFVsWlRqZHRZeTdVZXhhcFN1dmlSNUQ2ZXJVMlRLRWRSbmVVVzZ4?=
 =?utf-8?B?WDFnbXhzSHAyQ0hrL0laWTFJaWNUbGJUSHRhVFNwM3dUdUtYbkxyandzTmEw?=
 =?utf-8?B?dEc0T2NyMnI2WjVRQzZ2TVYrWXdvQ0ZlaUFMaTVPbEg5VUViRm1Jcy9SZkxm?=
 =?utf-8?B?SHVZV1g3aFdHUEJ3Yy9Sbm5hRG0wQXBLUlZjSE5SaXhXSnVVNzRVbzY1Z3lX?=
 =?utf-8?B?WXkrSk96Sm1hNVhWdXR6Q0F6RGVQdFViNUtlVWJDYU5JZ01lWFptbEhsbHFS?=
 =?utf-8?B?NS9aMDdiT1RLMkNrU2FGdkpybHhNR0ZEK3dIcXdKbHB1UmErUE1RNjVoNXQx?=
 =?utf-8?B?QllrR1ZkNjFQTzR2M2lxZzQydlA1UDB1Q3ZwdkdGYXY0OGNtYlo5UVVoaWRp?=
 =?utf-8?B?N0M3bm4xd0VJbXJCWmJJN21ESnRxRlBuWmdvMW5WeEc4dGZnZDZFNENrY0c5?=
 =?utf-8?B?R3IrTTYyZTdqWGRDRDhTZ2J0MmRtUllnamZmanEwQmtVajh0MU1rYzM0L0V3?=
 =?utf-8?B?YU1kWm5XUTF4M2tCakd5M05nOGJYVXFKN2lIK2t4QmVodVdiSGpQS3MrV0RF?=
 =?utf-8?B?Q1Q2bWwzbnVzd0JOUzVHWlpiWTY4YTFIQ1dXalJwN1ZNUlBKL0l2OVZxa1Rl?=
 =?utf-8?B?ZitjVm1zWW1LZytuYnNZUzNqSWRCMWtVbFYzdCt2dWRpNjdva28wbnhsa0sy?=
 =?utf-8?B?MWdXUEF0L05nK0M1OXRlNWU4YzA5b3A3Y1ZiRWUvVSsyRnlzK3lmZ0R4WFNi?=
 =?utf-8?B?aWxQbXF0bG0yMERvVitFYnBPZm84endlVjZhYlFmc2J4NHhacmxCYTR4YU8v?=
 =?utf-8?B?d1hRd1EvUnBTNllhcTVIMlJHdWJRTFlFVW1xbG5ITVJLdFZ2YjB5Mm1lU0hH?=
 =?utf-8?B?dE14TzEzb2NwL2ZqbUtZQ0hyWVBpbXMvVmRJOGdvTVhPTzV0Mm1nWjVuTU9P?=
 =?utf-8?B?eEE1WmlZakg3aEpaU3hkaVdiZkJGem5wbFdpbVFmZGdPVHBBekdicnpLbHpW?=
 =?utf-8?B?eFFWeTU5REdrZ0QrcGhsc0lPWVdxdW1JMVd1bU8wUEF6dFNJNTdvdnZXOXov?=
 =?utf-8?B?ck1paU1UQXNxNkZJdUt1ZXBlSFh3Wk1rR1RvcnFOTWtkaGFVZEp1RjVSTGxk?=
 =?utf-8?B?YnJqbjJNbWRkTnU1dnhMdDVsMDArU1QrcFRLTEFBMmh3aTloeHJ2SEc3QVBV?=
 =?utf-8?B?WGNFbEN3d0s5UW5uWVczV0dlRGNtYWpiUTdpcGlob2ZiOU1Md2FNT1ZSczBB?=
 =?utf-8?B?MEZEZkN4K3dlTW9INk8rNW5tdjlsMkViYmNjdEVMUlQzaHJpMitQN2lHbkR0?=
 =?utf-8?B?Tk1QcmxTc3JCcDA4Z3lGMGt3OHYzVzhVQnA3YTM3VmR2NnlBdjFiSmE1cnpi?=
 =?utf-8?B?Ly9qVGRFUHUzNmJRazZYOW1xaDMvZGxmT1JuQWZjSFVJWmNNL0luWFRqcmJ5?=
 =?utf-8?B?MTlIU1d0eDFqNmlpYnNpek5ETURWcWhJN0MrckNEOUhJSnd6L21mVHlYamFF?=
 =?utf-8?B?Qmc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	inbrgdyi4PRf9Tz8wpwT5dewzw1hdf9cwHsllBOgGfG6NVzJsPPiedjYrbjYJ7ODiQ9tVmn4DxBxrN0XiqJM1Ux2+v+S9nwzxUZTPkFzN8e5+0/EUloYxie59Tu/+CMjVfRwJP+wTnkxD7R81o0meos7oBpWDrquRO38Nb3/v7bj+dkTHAxHPdyU0JPNBgN1ST0Eij6xJhSKIJH+81SsWUV/o+xZSb0KxFrulaTeIsEDolb7ON+vH69J6bG/UfBU5FWigGlaGTbR4eRzsPsttO2QEy406Ph1njb6imtCW6LLyzhvHSjYxb7KZyjc+8RMfs3bF/UnuRnA/CRaWfzGGSFyNEijK5Xj7/BwKFPAVTWupeuhLMOawms16+GYS5i3fbnQ0wIpMBq3umRXwgdzJAioZxGP6A95MEw43IvZTaLx4GGSbSkulzPMBEN1bOGSDA2eAKzLFqNFdT/jwUxe9BLHJ3QgsqwzW4uTduuv4ST1AgpoilvU+GleOnqI/j5EuNwE/5xRsuohBtaXcvb4/CByXW/nhI7TrOsyKM/m1nJrsCKx3nydTT445ADjprbRSDUgNgSqnVtnDo9h7zYVmr8rrZ6tS+o2MbZGDIHz5jM=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3fa89a70-9dc2-4aef-6509-08dc2fdab167
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Feb 2024 17:05:50.8490
 (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: d+fsBdJrtzA8+JveSpLLjcxD+uQbB9Z6zjXdCOA49kHeOQjTb36BTEKC9sgOI8CzfLFU/NAgij9OhQTI3212ng==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB5157
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-17_15,2024-02-16_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxscore=0 adultscore=0
 mlxlogscore=999 bulkscore=0 malwarescore=0 phishscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2402170140
X-Proofpoint-ORIG-GUID: OCSbK3cY0FdtH0zUlGlHg_QC1zn2cNWS
X-Proofpoint-GUID: OCSbK3cY0FdtH0zUlGlHg_QC1zn2cNWS

On Fri, Feb 16, 2024 at 06:57:16PM +0000, Trond Myklebust wrote:
> On Fri, 2024-02-16 at 18:25 +0000, Chuck Lever III wrote:
> > 
> > 
> > > On Feb 16, 2024, at 1:19 PM, Chuck Lever III
> > > <chuck.lever@oracle.com> wrote:
> > > 
> > > 
> > > 
> > > > On Feb 16, 2024, at 1:18 PM, Trond Myklebust
> > > > <trondmy@hammerspace.com> wrote:
> > > > 
> > > > On Fri, 2024-02-16 at 08:33 -0500, Chuck Lever wrote:
> > > > > On Thu, Feb 15, 2024 at 08:24:50PM -0500,
> > > > > trondmy@kernel.org wrote:
> > > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > > > 
> > > > > > Commit bb4d53d66e4b broke the NFSv3 pre/post op attributes
> > > > > > behaviour
> > > > > > when doing a SETATTR rpc call by stripping out the calls to
> > > > > > fh_fill_pre_attrs() and fh_fill_post_attrs().
> > > > > 
> > > > > Can you give more detail about what broke?
> > > > 
> > > > Without the calls to fh_fill_pre_attrs() and
> > > > fh_fill_post_attrs(), we
> > > > don't store any pre/post op attributes and we can't return any
> > > > such
> > > > attributes to the NFSv3 client.
> > > 
> > > I get that. Why does that matter?
> > 
> > Or, to be a little less terse... clients rely on the pre/post
> > op attributes around a SETATTR, I guess, but I don't see why.
> > I'm missing some context.
> 
>    1. SETATTR is not atomic, and is not implemented as being atomic in
>       Linux. It is perfectly possible for, say, the file to get
>       truncated, but for the other attribute changes to get dropped on
>       the floor. NFSv4 communicates that information via the bitmap.
>       NFSv3 does it using the pre/post attributes.
>    2. When doing a guarded SETATTR, if the server returns
>       NFS3ERR_NOT_SYNC, the client may want to update its cached ctime
>       and resend.

All granted, but I'm still not clear. Let me ask this a different
way.

As far as I can tell, it's always been optional for an NFSv3 server
to include pre- and post-op attributes in wcc_data. Both the
pre_op_attr and post_op_attr XDR types start with an
"attribute_follows" discriminator. Therefore clients cannot rely on
receiving those attributes.

The patch description says that "Commit bb4d53d66e4b broke the NFSv3
                                                     ^^^^^
pre/post op attributes ...". And I think you also used the word
"nasty" in an earlier email. So what is broken if the server /never/
returns those attributes? What are the application-visible effects
of the server behavior change in bb4d53d66e4b ?

I don't have a problem reverting that part of bb4d53d66e4b, but
"broke" is doing some heavy lifting here. I want to understand why
we need to revert. Because it seems to me the server's current NFSv3
SETATTR implementation is spec-compliant. As far as I can tell,
bb4d53d66e4b might result in a little more network traffic in some
cases, but it won't impact interoperability or outcome.

Do you mean that you want to restore the previous, more optimized,
server behavior to return pre- and post-op attributes when they are
available? And if so, what is the application-visible benefit?


-- 
Chuck Lever