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;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Neil Brown <neilb@suse.de>
CC:     Jeff Layton <jlayton@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 11/13] NFSD: use explicit lock/unlock for directory ops
Thread-Topic: [PATCH 11/13] NFSD: use explicit lock/unlock for directory ops
Thread-Index: AQHYohNX7cvCckGh/kqPxDxsXdi4Iq2T38yAgACwrQCAANtGAA==
Date:   Fri, 29 Jul 2022 14:31:55 +0000
Message-ID: <B445DD2E-D356-47BC-8FD6-852565A867E4@oracle.com>
References: <165881740958.21666.5904057696047278505.stgit@noble.brown>
 <165881793059.21666.9611699223923887416.stgit@noble.brown>
 <6221A20D-6623-41EB-AC9F-BEFB1F4ED925@oracle.com>
 <165905802651.4359.17617640232417036364@noble.neil.brown.name>
In-Reply-To: <165905802651.4359.17617640232417036364@noble.neil.brown.name>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?hziXEmaWCpXz0TzrRwm+1Ugls5H02G1VeM44Rh4vFv3OrLyIDZVRFPkMiAKv?=
 =?us-ascii?Q?zWH0k0qO/oWJiq8OBXZ7P9UzA3n8/oK8luQTMAJSty0yfLI3yunNqGDiIDJh?=
 =?us-ascii?Q?HNhPvWVX4lN2h1WIN8Gr/psFM1XFfKihWlJftQIN/JRTUdHPnXb2X/tqVyiZ?=
 =?us-ascii?Q?BsZHLdoiEIuTZE+7b1RWQ7FrpAskRoqQ1QzssPnJmF7ouQgXlQZ0/Qt23It1?=
 =?us-ascii?Q?Bo/H/iu4QTkgSHMrML1P4mRcRsZ7ZKkOj4rSKri5/gf3yQFJIjrqYEPlqBwn?=
 =?us-ascii?Q?AWpGz3nHdXRQSoCEAouJos6Tlcbpkj1DfDjuc2d/xxinKq/xbvVz9ZU/xloN?=
 =?us-ascii?Q?QPIqLWdLAOxyxz1bno9+0OrdU8P1wW4bhPU8h8/Y77vOamLrytCNk/9LIIcV?=
 =?us-ascii?Q?ysqmSB+eKKtE1YtKEEY8o5gv68/Hz1PRq6uX7qzLVD6LbQO9K+QH9qinpmVS?=
 =?us-ascii?Q?r0O7U7P4brFfH4nSfgyLCpaeYMMOuGBCtkO2C/HyXE7rjwZKAaH0Wtm8JegE?=
 =?us-ascii?Q?z3XiQDbY80XWjtVN0L3rCOimx7li+3Eq2KVXoJEx0znJ/1lGTW35YzIwGMX/?=
 =?us-ascii?Q?R9rWLA+2TNwKWXwG/UkoDlt07OzgJ3zxCV5xhvqlKjxcIMBhKNawO64sGLwk?=
 =?us-ascii?Q?eiqz7dahS1A31SEL83BDybitbc0nxmYz+DWcgUE9065auIGOjlUm0BhtI+2O?=
 =?us-ascii?Q?0Cpf7Plwj++EMnbIT24wZfqTAqwloyPjZplPALyr8sVnveoLKAXdET7I+CgU?=
 =?us-ascii?Q?WO5qLjEVdlRxxsIEuJ2YEO0W6I9KFWq7MhjOiYRIipU2fNC9+xlzE3fIVtg0?=
 =?us-ascii?Q?09NL6HyuRv1vl/sM492qWH1VtjoBVWWPMdOp132sQoom/psvuG1zCXyvplRi?=
 =?us-ascii?Q?/7eD+BPobsYL3ObQpDSXl9xuoumzizWH/uvrUX+/wc6oXPrNJmT2J1ksZY8U?=
 =?us-ascii?Q?vtb80hhJHj1e9tC2IiJDjekykmA1H4t0aCpBGqlWgsUj1cqhNgNoBCIwo9EO?=
 =?us-ascii?Q?V1Onk8xmqYdEMyOpr50AJGuWij2f5arqWtK5NyjvUR9cfRjV7Q7w6XmGvE3E?=
 =?us-ascii?Q?PW0F5h1tMKkFgxJWqiAZ4CaS0Jft848T0pr/FT9tgSBf+mCnYs+uebQbUpAl?=
 =?us-ascii?Q?qTKY9l/jeh8UnC8GBN/w9jHgRmyb4dIKkkRewbL5hr3gtIMixgCix+gQnlb5?=
 =?us-ascii?Q?zbwwbs/T6M4voAHwBzSHFN5tZUMn/ssnWwcKZDPIs6p+HJGIBcnY9JZGKjHz?=
 =?us-ascii?Q?e+0ZbXhwf+2yUs/Qzdq8DwQlWfmMTljarM1aJC5YQXS1RuExmlsMTP9JnW4A?=
 =?us-ascii?Q?qlLxfOPEcXQoR/YUqVMVg+A3EfwIJZN3KeyeRNtO8R7Ulz8X7WsVeGrYlEI1?=
 =?us-ascii?Q?0cfgsT/G1v96pITsC/B9ie4V87GbLcH20+Tw6Lqf2Q1b+X6rd01/xHOTt2h2?=
 =?us-ascii?Q?5R9Gz20FKwtk0uMOYaiT/SAHydRxCkGmseDQ26wjcDxZhM8cfUUYGM/SSIq7?=
 =?us-ascii?Q?CxgJUT347+CmZi6vXmawDkcadiCIG+CsNnudZLpOGLJFkkd9M9fSKRZC6t8W?=
 =?us-ascii?Q?I7lCHvu5LU6BrjTxEn+JqmIQzj9HxSujI93tn7NTy0QOZJ2jldZPubBNLcqJ?=
 =?us-ascii?Q?lQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2083F3E7FFB39E458CA48CF255CCE32C@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: cbb14a86-4800-4269-951c-08da716f1619
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Jul 2022 14:31:55.1805
 (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: 61cxmy8twReJiyyvFxv04LbJwBrN/dXQH9ishS3VW2Iig0lefyc4un+IRU7YUkK+TrsEv+IvRuUzPuHUYT4iFQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB2725
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-07-29_16,2022-07-28_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 malwarescore=0
 adultscore=0 mlxscore=0 mlxlogscore=584 suspectscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2206140000
 definitions=main-2207290065
X-Proofpoint-GUID: 0ufHP74_OG8toRv-Grcre-_pnZKsKWtY
X-Proofpoint-ORIG-GUID: 0ufHP74_OG8toRv-Grcre-_pnZKsKWtY
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE 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 Jul 28, 2022, at 9:27 PM, NeilBrown <neilb@suse.de> wrote:
>=20
> On Fri, 29 Jul 2022, Chuck Lever III wrote:
>>=20
>>> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote:
>>>=20
>>> Also move the 'fill' calls closer to the operation that might change th=
e
>>> attributes.  This way they are avoided on some error paths.
>>>=20
>>> For the v2-only code in nfsproc.c, drop the fill calls as they aren't
>>> needed.
>>=20
>> This feels like 3 independent changes to me. At least the v2 change
>> should be moved to a separate patch. Relocating the "fill attrs" calls
>> seems like it could cause noticeable behavior changes, so maybe it
>> belongs also in a separate patch?
>=20
> Three intimately related changes that could be applied in sequence.
> What would be gained by having separate patches?
> To my mind, the primary issue is review effort.  Mixing unrelated
> changes make review of each change harder, so keep them separate.
> Mixing related changes is less of a problem as the abstraction that you
> need to keep front-of-mind are fewer.
> On the flip side, every patch added increases the review burden.  If I
> wanted to move all calls to foo(), I wouldn't have one patch for each
> call.
> I think that patch is easy to review as it stands, but if you think not
> I guess it could be split.
>=20
> Allowing bisect to isolate the problem precisely is another reason for
> keeping patches small.  If a bug were found to be caused by this patch I
> doubt it would be at all hard to localise which part of the patch caused
> it.

I adjusted the patch description and left the content as a single
patch. I was initially confused by "drop the fill calls"... the
patch is not physically removing code.

--
Chuck Lever