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;
Date:   Sun, 30 Jul 2023 12:55:39 -0400
From:   Chuck Lever <chuck.lever@oracle.com>
To:     Hugh Dickins <hughd@google.com>
Cc:     NeilBrown <neilb@suse.de>, Chuck Lever <cel@kernel.org>,
        David Howells <dhowells@redhat.com>,
        Jeff Layton <jlayton@kernel.org>, Jens Axboe <axboe@kernel.dk>,
        Matthew Wilcox <willy@infradead.org>,
        linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-mm@kvack.org
Subject: Re: [PATCH] nfsd: Fix reading via splice
Message-ID: <ZMaWC23FXQ/C4rng@tissot.1015granger.net>
References: <169054754615.3783.11682801287165281930.stgit@klimt.1015granger.net>
 <169058849828.32308.14965537137761913794@noble.neil.brown.name>
 <ZMaB1BwBfNko1ZoE@tissot.1015granger.net>
 <3082a8da-4a13-de28-ed50-8aa2e7a59afd@google.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3082a8da-4a13-de28-ed50-8aa2e7a59afd@google.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?0l5C2GtEcU11mu03OVLZRSnVCZ1WkKleUwbUexR5UYwl92SEqabGGne1eu+0?=
 =?us-ascii?Q?vwQg7gTcCNsZdI1pnUKySGqcUG+cOFxSPO1qud+/B7v6yUYhxdIhlDsn0Dv9?=
 =?us-ascii?Q?A/iLKWG8PLWTQa/Frn1CgIH9abt8dtY2lQ25FQJ6osKB9dM9nCThOt0/rg9y?=
 =?us-ascii?Q?d0YN8+8tiv8ckV7y4RdUS1k3cdoG9/9AwppeRNOIbrzqApjz1csXXd8tVVF+?=
 =?us-ascii?Q?1vZIBnc+N6dZOKZjJKxznt+HeyKTrxQVnHp+T3o1ERRZnw9eIneSnBCKrZzQ?=
 =?us-ascii?Q?N+yOaHcolXk+/RswRowD4X6PSTBATZb/Qi989nM+YzWW3zuZCEntc8Bfgcye?=
 =?us-ascii?Q?Fu191uYBM8BxBh8oPlFqKhVs1xteG2Dvkq4HmD6jFaZJ6lLSHT9fcHtZ6bOS?=
 =?us-ascii?Q?43sly8xGjUqgYJ6a09YI7prYv1n0WR2m4Ka7fw5NABwJJ99d4aDj+ypOv4/B?=
 =?us-ascii?Q?iSN3M2tmkijkY5r5ge2cxs08/bMhs/8ERnU9GnVT16Rm5J/owELuvtR++CAl?=
 =?us-ascii?Q?NiDxElUKWAbYSuiLO/1JmYHaR8D1jiYe1kWBTBbFAAZAyO2dFSG887coHB9c?=
 =?us-ascii?Q?o64/+rU52iDMn5oqm4dv8sGzYHlO1ThzkJ9vR2u8S9WCrST0ZQ/cZTX5QfUq?=
 =?us-ascii?Q?WtUjGaBGktm8DGr8WWmOUEJnRyc+7Jy5W7cgc8sAtskNfRVwMqdElZf6FVLA?=
 =?us-ascii?Q?1MJQemTX05qa0wzgD/hLy8vNPb59g/9CfKtT1iruPZCctQ/YSy5ooO9bl2V1?=
 =?us-ascii?Q?BhIJBitwii/lp2J6pbAoHkGhzhB/HhsnUfXFDkIADopzCnoo5hC+A8d9cUCP?=
 =?us-ascii?Q?5Wj/vIs3nDGEADKA3IXoBDfnZdzNljqBI5RNoADKgYm9TjV9Vxsk4043zA1d?=
 =?us-ascii?Q?bCeixaXOPPFs96h/em4xoBYmiBBWjSvbPUUJjcNDjqCGkNDLOqqFNO43c5mc?=
 =?us-ascii?Q?KzpRgcP7GkKJNNLxKbtVq0kYvytGH7I7jzIzUABlK/gAEKEfXqrXQXxVDLdG?=
 =?us-ascii?Q?TUYbu+ZCN26zNIwIl+ypy6Lg4dAcw3Tf0ULgdHCCvO0NEnTpJz+rmYY7stme?=
 =?us-ascii?Q?sXjKw3wKQCfMUezsjXIxyoe0Rw2thFtTD92UhwR1ZxCX6lUQb/C5oTujIgR7?=
 =?us-ascii?Q?GzbhSBPBkUjhvqHtWafYTZRBYGC2hdHQHIbw/ZTonu3KFGgdGt2RMxhgzK/Y?=
 =?us-ascii?Q?Q1s7SBsn/o5fBDHjFNqznNrIjXDCGR4YO3VCEjJ1HVijDt2J/xxfegYTh6gW?=
 =?us-ascii?Q?9Gp4BRKX90//rcrISoIO/Xczkb12ArAnxp6dzo+g6AB5DTRHN9G/QvSsGDb4?=
 =?us-ascii?Q?k4rBx6QzL8kTpDfaxxHBrK02eOOZJRLT2IKbs0MW7OdlUqEN3Y4h3Zm1iwL5?=
 =?us-ascii?Q?zqy36poZVKjFzM4KhB6TAlQb2Lclk1XkupROeUtSmVqkaB6nr7yvKRUU4CB/?=
 =?us-ascii?Q?s/0802YAYqD1kgyLRQncRlQ0lBd5a4UF38jGKnlma8b8lORw6SAknTdJMsyq?=
 =?us-ascii?Q?xofECbhJDOv15afwYjBVjms1DkDH4BCTbq619hXYsl67PiKnp8gbcHt6u4jR?=
 =?us-ascii?Q?069AwPXJn1LIj75O1iZj1dM8RGX6a8YOG16hR1hhZM5LkocSzztgjNVACzHh?=
 =?us-ascii?Q?yQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?us-ascii?Q?QCHSe3paoOL9jKD+9GqqeYftecly04ASfjUwgdJXlUMJSTjVooVnQLfZ1RSd?=
 =?us-ascii?Q?YEjtCUINjexJc8vqxlvPe7T9EDCqpCpYGtTlQlYtXrAbgx3S9t1xKFIj6PI8?=
 =?us-ascii?Q?HzA9igM5tiBy33rEBE/DqR68m6C7ANwLasNazl0llQp/zjF1gB+nZ12SBk9m?=
 =?us-ascii?Q?9wYDCcFrSAwmmrdH9WE5Od+ipfeAyPha/tn0eH2wiLhdPUVPFiqkJE3NfZZC?=
 =?us-ascii?Q?B2mbrX/GbugGJzHhiaGIil56quRWewM4aULaEpI3jZvLE/eYkKi6vR+YdJwW?=
 =?us-ascii?Q?9uyV+sBZNhrosSYXSnCLCoJw7Nqzyhq3hOQmdE+1P0VUO/KOY9xBokKtQ4We?=
 =?us-ascii?Q?y7u0boGKg8W7AXm6zi680UTZkEXq+wPNXPHSTpvNY/dQEvIz12szZSgQNXaH?=
 =?us-ascii?Q?DVvh7HTP3CrILKj/eOhn5ZzYIahuDUOM6k+mIORTCd8SpRdp2OAJAs82hw9/?=
 =?us-ascii?Q?Xkm2rl6AAGBtsW3jUNnMQnFmANrpqTehfTb80bIIOqSzZ3Gf8ct1vwzXjfP/?=
 =?us-ascii?Q?pMRXrMngkL3FXq4+dOhat2lFAwIpu5lLFsR8F4nynF92BBpPG6mo+LH8Z/xg?=
 =?us-ascii?Q?YR+k7I44PqXFcriMQE2tLgaMOcS4kFt7M/tEEdfyVmNo7smKQDo7mc6U8F45?=
 =?us-ascii?Q?B2zD//T9UGtqkhugcQiHikoZ2/Zz69AMIorY1na6o2VON0xT1k6skZAG3wWY?=
 =?us-ascii?Q?38pNZPAoKcl6OowY9BpP3qK3ipo753ow5sO2jCH6Ar0LjDI6TWeSTWIMa/XS?=
 =?us-ascii?Q?KPyhz7C9041Kq9ztpfBrugmyt4L2DIsUW3+TYs+rSIrAFiEhdpeLIJtEoKOE?=
 =?us-ascii?Q?P1whhvieUprKS+FHtr8opLs69jiHBi+/iok11xO6DELzbwKdTCaNqPP+ZLTu?=
 =?us-ascii?Q?r6+LMBUkjHHWGsjNUMM2/Yq3AgUmZqX2+SKHXKL0SilO2Jsk6cXG1o+lsXaq?=
 =?us-ascii?Q?OhWhBgvnBI4f72RuA79DKg=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8d011b71-3135-46ca-c631-08db911dcf71
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jul 2023 16:55:42.4826
 (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: b1IHx3uWU3sMdv7CzoBz5cpUwlC9EDybnUg1Quo49LXfIZNU19KDoYW8Q8d5hxp6u1VDI5vYl850SFf5Z4pk4Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4679
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-07-27_10,2023-07-26_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 mlxlogscore=943
 phishscore=0 mlxscore=0 adultscore=0 malwarescore=0 suspectscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2306200000 definitions=main-2307300156
X-Proofpoint-GUID: PuLXP-tAd48jHwPpgPSSM5k684fMvCeM
X-Proofpoint-ORIG-GUID: PuLXP-tAd48jHwPpgPSSM5k684fMvCeM
X-Spam-Status: No, score=-3.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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 Sun, Jul 30, 2023 at 09:50:44AM -0700, Hugh Dickins wrote:
> On Sun, 30 Jul 2023, Chuck Lever wrote:
> > On Sat, Jul 29, 2023 at 09:54:58AM +1000, NeilBrown wrote:
> > > On Fri, 28 Jul 2023, Chuck Lever wrote:
> > > > From: David Howells <dhowells@redhat.com>
> ...
> > - This fix is destined for 6.5-rc, which limits the amount of
> >   clean up and optimization we should be doing
> > 
> > I'd like to apply David's fix as-is, unless it's truly broken or
> > someone has a better quick solution.
> 
> I certainly have no objection to you doing so; and think that you
> and David will have a much better appreciation of the risks than me.
> 
> But I ought to mention that this two-ZERO_PAGEs-in-a-row behaviour
> was problematic for splice() in the past - see the comments on
> ZERO_PAGE(0) and its alternative block in shmem_file_read_iter().
> 1bdec44b1eee ("tmpfs: fix regressions from wider use of ZERO_PAGE"):
> ah, that came from a report by you too, xfstests on nfsd.

Yes, I thought we had visited this ZERO_PAGE approach before, but
couldn't put my finger on exactly when or where.


> In principle there's a very simple (but inferior) solution at the
> shmem end: for shmem_file_splice_read() to use SGP_CACHE (used when
> faulting in a hole) instead of SGP_READ in its call to shmem_get_folio().
> (And delete all of shmem's splice_zeropage_into_pipe() code.)
> 
> I say "in principle" because all David's testing has been with the
> SGP_READ there, and perhaps there's some gotcha I'm overlooking which
> would turn up when switching over to SGP_CACHE.  And I say "inferior"
> because that way entails allocating and zeroing pages for holes (which
> page reclaim will then free later on if they remain clean).
> 
> My vote would be for putting David's nfsd patch in for now, but
> keeping an open mind as to whether the shmem end has to change,
> if there might be further problems elsewhere than nfsd.

I'm open to that.

-- 
Chuck Lever