Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 28 Jun 2023 19:23:54 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Matthew Wilcox <willy@infradead.org>,
        Sumitra Sharma <sumitraartsy@gmail.com>
CC:     Hans de Goede <hdegoede@redhat.com>,
        <linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Ira Weiny <ira.weiny@intel.com>,
        Fabio <fmdefrancesco@gmail.com>, Deepak R Varma <drv@mailo.com>
Subject: Re: [PATCH] fs/vboxsf: Replace kmap() with kmap_local_{page, folio}()
Message-ID: <649ceb3aeb554_92a1629445@iweiny-mobl.notmuch>
References: <20230627135115.GA452832@sumitra.com>
 <ZJxqmEVKoxxftfXM@casper.infradead.org>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZJxqmEVKoxxftfXM@casper.infradead.org>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?wMg/5txX8tdiY07zau27T4/FoiaS8DoXhGZ97aXECA3wQIrh3E2x40/8tvB3?=
 =?us-ascii?Q?sGzvv6XdUgb+rIJf6JoojH+OL7T7j8A16rpNvPZtEcgvDEVh/+c1U0rUU9p9?=
 =?us-ascii?Q?jyDj07gn4hYwfbDsvtkKmEp+gFNl9uoQfDoTyacQpNVhV9pqjC3GFDnhZ5Zy?=
 =?us-ascii?Q?P2wQg/UormSzwrfIBBVCuYW1vC/jOL6btJfKuR13YhulzFuKAyMnwhQUa/8t?=
 =?us-ascii?Q?PwLPPapHt6ztKL/ApLfpH6/iE1wjmvgSqKsHKdssLOVZ/RJTXhh36FO3gSNn?=
 =?us-ascii?Q?gdAO/Ix2+C2v8IJByyHVwDQDCoj2O9+w5jM4GjZEGGVnYQtZHda4ce1K/Imj?=
 =?us-ascii?Q?evvxk4rIOASvwG+Ff2Fkpawv0xOqKYgnSgM+oItRVS5u7Tk+qat/BF/sCxje?=
 =?us-ascii?Q?u4Fo0YiEtJ7Lt3yAb0eQtkSItgFpCLIH3SDWX1f2h2zl+eqZjk5QHTRPlSag?=
 =?us-ascii?Q?HLMwqHfyabta0gwK5bM1qCSzek1sr6E1pCr9HYgIioPi1VRmU+2JdtvgZ8bG?=
 =?us-ascii?Q?4PBvaHfsi4sU4kQ1tbbQqOeX45PYPtaGig4wMruisJFweGNih5RyF9ma7OND?=
 =?us-ascii?Q?NPre7fCPY2SItSmbAiEXhVHC6IsAOmAFMeZT4P+agGP3XjzhcvLoXSQ/iCms?=
 =?us-ascii?Q?BE/KlElNRkI3hCf1fV+Zrd4pNqSVBGZBZyYJOYU9gVAM7PFKkuSf5VdfjIxj?=
 =?us-ascii?Q?oQjZfFoQgNsHczrP6H6fQqTdXf3KfHo72DGuC3WvXzmbZGg2xKvnzlamD9me?=
 =?us-ascii?Q?pyKxQ2TEPhVbZqL8SpRLVNJMXyAcjXLo3ZxYsHmiELBHnw+Q7HJ0Hz8r/mjh?=
 =?us-ascii?Q?O6j5kJljcZE/QK2hAcxNAdjzQcXIp95f/6smJWi1jf3RISIHyGd9ojrihzt8?=
 =?us-ascii?Q?oWdODDqWFIWE84C88aIpJTg67u7CbEW7OUMEzui47JnTjEj2GhXs/NUDUVDM?=
 =?us-ascii?Q?E9bVxb15A9mlUkLGhaKneO10Q2Y0Rd4j1pMTfLO2zWko1K2767HQKfX3gt6i?=
 =?us-ascii?Q?Y1BA228PVM9RkeTItRtOG+E7a5woGqw5TPRVVQe4xmJE6tQ+elZ1IH97qs3Y?=
 =?us-ascii?Q?ggoDPt6uwF3E90idIBZKc9moGk4bhxIsuJv7Ge8DJYp1u71/+5RY3T/48lEI?=
 =?us-ascii?Q?sMySGPvNCq1y/xBlSTiUWUDC1NZKwxniDqNCsUQ/MNCm2TDVIpZ5tFvJbaM/?=
 =?us-ascii?Q?CcwPBfUPC2dLZz6/p+Jj+CKx8G3IIJb5HN/f3+AniuF5MHBTBKTY4ckc3k3N?=
 =?us-ascii?Q?WNw5uxJ0frORr/OpB8MK9X1bEuMKmhJUGckH+/p8kE24Qt6nMt07wdDkJCar?=
 =?us-ascii?Q?NQz7T8jFcuvFEle+zPeuY2UVC5ldaUOm72j7HitLpAG7WqYAm3NOLqggntWn?=
 =?us-ascii?Q?Bw4UaDKPR1+FxtdiiPOjMSzNvbGuykS9qE3bAj/K86MjT907zvKeKUkyJJzI?=
 =?us-ascii?Q?5RN0lMY//TA2+WWZ54lP76w4I0n2ofufE8DQmkrqGx9Nu5S64ijJlGZU8CDF?=
 =?us-ascii?Q?SDhrMn6pXwtjpL1FWehae+pWb2ajCWre17vm/RNLdETSiRJpleSjT7Om9Ars?=
 =?us-ascii?Q?ibn1TkpyIMnFxSwJ4W99kUyFLOoo0+6YxH9ySNO/?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 99fadb08-2a05-4f8f-40ae-08db7847e5de
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jun 2023 02:24:00.1156
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: yG3/s8zIZFP77qH9L8r8+7QURRHU/QuQyDF41jgi3CCXClI5ZN7JW7io7fz8hy082RaIAlalgaZM44PNgjh5kQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB8122
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        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-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Matthew Wilcox wrote:
> Here's a more comprehensive read_folio patch.  It's not at all
> efficient, but then if we wanted an efficient vboxsf, we'd implement
> vboxsf_readahead() and actually do an async call with deferred setting
> of the uptodate flag.  I can consult with anyone who wants to do all
> this work.
> 
> I haven't even compiled this, just trying to show the direction this
> should take.

I'm a bit confused.  Is this progressing toward having the folio passed
down to vboxsf_read() or just an expanded vboxsf_read_folio which can
handle larger folios as well as fix the kmap?

Ira

> 
> diff --git a/fs/vboxsf/file.c b/fs/vboxsf/file.c
> index 2307f8037efc..f1af9a7bd3d8 100644
> --- a/fs/vboxsf/file.c
> +++ b/fs/vboxsf/file.c
> @@ -227,26 +227,31 @@ const struct inode_operations vboxsf_reg_iops = {
>  
>  static int vboxsf_read_folio(struct file *file, struct folio *folio)
>  {
> -	struct page *page = &folio->page;
>  	struct vboxsf_handle *sf_handle = file->private_data;
> -	loff_t off = page_offset(page);
> -	u32 nread = PAGE_SIZE;
> -	u8 *buf;
> +	loff_t pos = folio_pos(folio);
> +	size_t offset = 0;
>  	int err;
>  
> -	buf = kmap(page);
> +	do {
> +		u8 *buf = kmap_local_folio(folio, offset);
> +		u32 nread = PAGE_SIZE;
>  
> -	err = vboxsf_read(sf_handle->root, sf_handle->handle, off, &nread, buf);
> -	if (err == 0) {
> -		memset(&buf[nread], 0, PAGE_SIZE - nread);
> -		flush_dcache_page(page);
> -		SetPageUptodate(page);
> -	} else {
> -		SetPageError(page);
> -	}
> +		err = vboxsf_read(sf_handle->root, sf_handle->handle, pos,
> +				&nread, buf);
> +		if (nread < PAGE_SIZE)
> +			memset(&buf[nread], 0, PAGE_SIZE - nread);
> +		kunmap_local(buf);
> +		if (err)
> +			break;
> +		offset += PAGE_SIZE;
> +		pos += PAGE_SIZE;
> +	} while (offset < folio_size(folio);
>  
> -	kunmap(page);
> -	unlock_page(page);
> +	if (!err) {
> +		flush_dcache_folio(folio);
> +		folio_mark_uptodate(folio);
> +	}
> +	folio_unlock(folio);
>  	return err;
>  }
>