Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Thu, 28 Sep 2023 09:56:20 -0400
From:   Chuck Lever <chuck.lever@oracle.com>
To:     Jeff Layton <jlayton@kernel.org>
Cc:     Alexander Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <brauner@kernel.org>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Neil Brown <neilb@suse.de>,
        Olga Kornievskaia <kolga@netapp.com>,
        Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
        linux-nfs@vger.kernel.org
Subject: Re: [PATCH 51/87] fs/nfsd: convert to new inode {a,m}time accessors
Message-ID: <ZRWGBGqYe3rF5CRY@tissot.1015granger.net>
References: <20230928110300.32891-1-jlayton@kernel.org>
 <20230928110413.33032-1-jlayton@kernel.org>
 <20230928110413.33032-50-jlayton@kernel.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230928110413.33032-50-jlayton@kernel.org>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?4Owavdr32Xlu5Z947XVcj+04DAaP1HHZ9GI/4pgHsD1AK5BYkDbA9XRwfe4N?=
 =?us-ascii?Q?dAt53L4TTN2EZEUy8AgRBA3W6omC63mu7QLRdpVEzQONzjYscdj1jQxY7b9l?=
 =?us-ascii?Q?7l0qa3QEL1jplj8ob1Yeio8JkI02ltcltwLoAQmRzqqo5ndGF5bot+fr7bnV?=
 =?us-ascii?Q?VVX4Br+yCFRXaGreHTNSoiXer7CWpQJ2arpraFskF61vVojbTKWz2ia4paLN?=
 =?us-ascii?Q?+u3dNcVeQK8AS72Z6qd+rcZxsZpKz/3pYNf/NwLr/PxKLREsphuPrXHWStO8?=
 =?us-ascii?Q?Pr7plB2fMfENOlJMkHm1EV8Dn/uTDManPyDAj9Cxwu45XfxpVLQkmjOcfrJp?=
 =?us-ascii?Q?5zpGsSsaMCDbSRV8dhI9DeCV7jn5AkaNYMrsgyjKfwgwis60x9yj8TzUEKv0?=
 =?us-ascii?Q?Bc+dsZQrE4bSFm5jm8IA/Dx7JYajynL6ph5We9GZNqgFFu1B3DgAjwD1d8hH?=
 =?us-ascii?Q?lz5ugvD2Wq8TnQmMCSqjq3Ur0X5W0FhPfLbhB1iTU9XvRV2K1IJoqr1776ku?=
 =?us-ascii?Q?RxRwdPiNnexJnxy3GXdqzgV7tAn2e4WGFG2VhX1QSO758O8QaX6as0GV1O3A?=
 =?us-ascii?Q?1/3bVIxD1XmYBPM5PJUFzRI9IsRYziUh99jHTevsgdrcqI/EPjrTQwSq+8Yk?=
 =?us-ascii?Q?DNg62HgZb++PfnqOtk9fF3gCsM2YmOhAxzTBkZH02fBobHydKCP7PSg/6lso?=
 =?us-ascii?Q?my8EEMJgXC5cpjJQzpBprxhLVD679IHmwXjdIsaoPHyfXo5xcVeLW4X3xFsU?=
 =?us-ascii?Q?rAVv5tGhQdeeJ9LDxMNHRbQnda5A4FcG9Sw8h+++Opjea9nut5p+t4s0qCUX?=
 =?us-ascii?Q?ygCllk62Zktvgr5v55GUB9InMvew58CPi/vtWD3WYO6jl1ePKMgHzm/94FOH?=
 =?us-ascii?Q?7UHzGzgY3RgNx1hnpNmtpFVq3ZTzWOnQoN1JWeVM3EX+seBGZiSi5H7SST6J?=
 =?us-ascii?Q?glU0mYPNF+iDTPUIkgLepx246oCwu+TuPHW/UhuKZXPj0Lk6p50v3hwile67?=
 =?us-ascii?Q?LaPpLyBy9Y7ls4dD4C4PsIduPEKUdgI/Seeg9nD5OME+RKjBaI2rSA7E90rE?=
 =?us-ascii?Q?0ZAQLYdQKy+c5/iGXFgrbQwC1KuZN4f3reQ9O5C1CooWqCvoRLWwJGiZxpKx?=
 =?us-ascii?Q?d3f4l2aN9flPpzuIl2x+s79G9uXwiPAbzrg+cACHoPGQsqOdcRbrtYrMZY0G?=
 =?us-ascii?Q?0YW5Ra7VStmIq0LOlEqTnhgK4WRtdw23xIRBxSc6Snl4Xx3Ju8ki0JaccZUb?=
 =?us-ascii?Q?5BPKq0FmnOWLP5GReYk9rEtO/3Abbk0s9rWboCJctow9vrUPNJC1Lgis4Vyh?=
 =?us-ascii?Q?VpOZpJzRufhkCNdjfzN93sCH5cGhG1zJ7P6KM6sDC+2rIztkXFnS4dLFvOld?=
 =?us-ascii?Q?vX6g3cu3qwg49Lm22lgk9oQxm4PRP0k2h3g0TeMrxzoa/rR6ec7wqBLr6nCh?=
 =?us-ascii?Q?H4m2AXugl2Ek7b3eZre5sQBmMi8tgim1d1hyF20+tbf7No44JNKZ/oWFq/cf?=
 =?us-ascii?Q?CXPf+SHoHN4mxmpPhUhcaBPMShgJ5Y2wG2rgRSGvIKgACGGxNkiHfvlWw5dH?=
 =?us-ascii?Q?tgz7QOQKiPty5BmMiaj8AtBASRSvkfopvTXZCxyNeI8hjqZl2rtMDR1LH5h/?=
 =?us-ascii?Q?XA=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: LBYzdiUdf4d8YnXAqV8q7345bCtxxn7n4bGaCkSNZYCsSNW+FcKfS/T57RPUH/SDG4UiPxGWU+xvTTP6t30PF7wKYLeKjwvYdAa9WBdbrKEpV064ydv80m+2dlbxvZDBF6p6lFlS9/xj56aznV898xp/l6HVwJ+8iX7qN4KsYdN6oKGn83jImQ7vmbv6s/hiC7+97K7SnRelHWG9sm8Ar9cEUeD+XmfFOMjqpALSvgFUlVSOh0YoPtR9+Q9P8OssnacOEuiwvEqBuAQufVR78+HUdc1yAiAFrSP/xs8nhnlerIdMrdgCUhrTrD2o4BcTX+TzurpE+wI+CoUoGjZ0cPYCRWTolkqJD/S3trlyPJU4qiwSdBhkFSm4wEBtttoRh44Z8MaF8Ieu8V8oCmLekgEr/LET+2eKhNgpqlWK+1nJ/N8hEkxiGnpkPLc2YS/nAG5FKgo+p6s1M+8S8cYH7t/OvvapyS+bjlfB8+yaYLD6KYiJwUCUTErM8/hHmyWYXTPlSzlXHtVEeszhJcHI/bJE9y3suW54Igs3A4NRYPG7hszy2EjdI+x0fPFBkYVr84I1TPBuW/nACs/g133xZBVWGpFTwqPFTfwlp2iEKrB3BJZzO+/ygLkqFgkclvJMOhf9NkL3lcxkHUdJKU5Rj37OounHUKdiBSia1Q9kB86rEEyS10/VKN/SkQjYBizWdUwSPoc73akCNoXVh4fmyJwSDOiicSe8ge4HPVmco60gn3TIE6s7dhD7+ORX7VuzM6yrasXevWhvnhi0fpz0oojQ3gOab3eFQssNz9RRDeEynb7wtN9v8o/B5+4NDTQL9djBxj6cDFn/KO000c9QpUepKa2fRHaKUqN7gdjMhMGstvlh3rSq/mBcCe+6r2ird4uCZRllclrmpQjxNJ902dfv0ZvWtOj7+pfcclvPohs=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2a2a495b-025a-4b33-28b7-08dbc02ab33a
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Sep 2023 13:56:23.2510
 (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: b/xaUmo4p/G5AAO5MJAeVDZaLUrSfsMAaClq7pgZGx4BNJOoWxKAocu4+HDRz3vCXmZnpJuw2qNTBEHFfALavw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB7156
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.267,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-09-28_13,2023-09-28_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 bulkscore=0 mlxscore=0
 mlxlogscore=741 suspectscore=0 phishscore=0 adultscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2309180000
 definitions=main-2309280120
X-Proofpoint-GUID: T_8elOJXk3TMZ3d89Q6GmSw5cdusYLZn
X-Proofpoint-ORIG-GUID: T_8elOJXk3TMZ3d89Q6GmSw5cdusYLZn
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,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-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Thu, 28 Sep 2023 06:56:43 -0700 (PDT)

On Thu, Sep 28, 2023 at 07:03:00AM -0400, Jeff Layton wrote:
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/blocklayout.c | 3 ++-
>  fs/nfsd/nfs3proc.c    | 4 ++--
>  fs/nfsd/nfs4proc.c    | 8 ++++----
>  fs/nfsd/nfsctl.c      | 2 +-
>  4 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
> index 01d7fd108cf3..bdc582777738 100644
> --- a/fs/nfsd/blocklayout.c
> +++ b/fs/nfsd/blocklayout.c
> @@ -119,10 +119,11 @@ nfsd4_block_commit_blocks(struct inode *inode, struct nfsd4_layoutcommit *lcp,
>  {
>  	loff_t new_size = lcp->lc_last_wr + 1;
>  	struct iattr iattr = { .ia_valid = 0 };
> +	struct timespec64 mtime = inode_get_mtime(inode);

Nit: Please use reverse Christmas tree for new variable declarations.


>  	int error;
>  
>  	if (lcp->lc_mtime.tv_nsec == UTIME_NOW ||
> -	    timespec64_compare(&lcp->lc_mtime, &inode->i_mtime) < 0)
> +	    timespec64_compare(&lcp->lc_mtime, &mtime) < 0)
>  		lcp->lc_mtime = current_time(inode);
>  	iattr.ia_valid |= ATTR_ATIME | ATTR_CTIME | ATTR_MTIME;
>  	iattr.ia_atime = iattr.ia_ctime = iattr.ia_mtime = lcp->lc_mtime;
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 268ef57751c4..b1c90a901d3e 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -294,8 +294,8 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			status = nfserr_exist;
>  			break;
>  		case NFS3_CREATE_EXCLUSIVE:
> -			if (d_inode(child)->i_mtime.tv_sec == v_mtime &&
> -			    d_inode(child)->i_atime.tv_sec == v_atime &&
> +			if (inode_get_mtime(d_inode(child)).tv_sec == v_mtime &&
> +			    inode_get_atime(d_inode(child)).tv_sec == v_atime &&

"inode_get_atime(yada).tv_sec" seems to be a frequently-repeated
idiom, at least in this patch. Would it be helpful to have an
additional helper that extracted just the seconds field, and one
that extracts just the nsec field?


>  			    d_inode(child)->i_size == 0) {
>  				break;
>  			}
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 4199ede0583c..b17309aac0d5 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -322,8 +322,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			status = nfserr_exist;
>  			break;
>  		case NFS4_CREATE_EXCLUSIVE:
> -			if (d_inode(child)->i_mtime.tv_sec == v_mtime &&
> -			    d_inode(child)->i_atime.tv_sec == v_atime &&
> +			if (inode_get_mtime(d_inode(child)).tv_sec == v_mtime &&
> +			    inode_get_atime(d_inode(child)).tv_sec == v_atime &&
>  			    d_inode(child)->i_size == 0) {
>  				open->op_created = true;
>  				break;		/* subtle */
> @@ -331,8 +331,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			status = nfserr_exist;
>  			break;
>  		case NFS4_CREATE_EXCLUSIVE4_1:
> -			if (d_inode(child)->i_mtime.tv_sec == v_mtime &&
> -			    d_inode(child)->i_atime.tv_sec == v_atime &&
> +			if (inode_get_mtime(d_inode(child)).tv_sec == v_mtime &&
> +			    inode_get_atime(d_inode(child)).tv_sec == v_atime &&
>  			    d_inode(child)->i_size == 0) {
>  				open->op_created = true;
>  				goto set_attr;	/* subtle */
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 7ed02fb88a36..846559e4769b 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1132,7 +1132,7 @@ static struct inode *nfsd_get_inode(struct super_block *sb, umode_t mode)
>  	/* Following advice from simple_fill_super documentation: */
>  	inode->i_ino = iunique(sb, NFSD_MaxReserved);
>  	inode->i_mode = mode;
> -	inode->i_atime = inode->i_mtime = inode_set_ctime_current(inode);
> +	simple_inode_init_ts(inode);

An observation about the whole series: Should these helpers use the
usual naming convention of:

  <subsystem>-<subject>-<verb>

So we get:

  simple_inode_ts_init(inode);

  inode_atime_get(inode)


>  	switch (mode & S_IFMT) {
>  	case S_IFDIR:
>  		inode->i_fop = &simple_dir_operations;
> -- 
> 2.41.0
> 

Otherwise, for the patch(es) touching nfsd:

Acked-by: Chuck Lever <chuck.lever@oracle.com>

-- 
Chuck Lever