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;
Message-ID: <e055b3e4-22c3-c0e2-fb8b-81ba8b97382d@redhat.com>
Date:   Thu, 29 Jun 2023 07:35:37 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH] fs/vboxsf: Replace kmap() with kmap_local_{page, folio}()
To:     Sumitra Sharma <sumitraartsy@gmail.com>,
        Matthew Wilcox <willy@infradead.org>
Cc:     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>
References: <20230627135115.GA452832@sumitra.com>
 <ZJsg5GL79MIOzbRf@casper.infradead.org> <20230629043031.GA455425@sumitra.com>
Content-Language: en-US, nl
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20230629043031.GA455425@sumitra.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Sumitra,

On 6/29/23 06:30, Sumitra Sharma wrote:
> On Tue, Jun 27, 2023 at 06:48:20PM +0100, Matthew Wilcox wrote:
>> On Tue, Jun 27, 2023 at 06:51:15AM -0700, Sumitra Sharma wrote:
>>> +++ b/fs/vboxsf/file.c
>>> @@ -234,7 +234,7 @@ static int vboxsf_read_folio(struct file *file, struct folio *folio)
>>>  	u8 *buf;
>>>  	int err;
>>>  
>>> -	buf = kmap(page);
>>> +	buf = kmap_local_folio(folio, off);
>>
>> Did you test this?  'off' is the offset in the _file_.  Whereas
>> kmap_local_folio() takes the offset within the _folio_.  They have
>> different types (loff_t vs size_t) to warn you that they're different
>> things.
>>
> 
> Hi Matthew,
> 
> When creating this patch, I read and searched about the loff_t vs size_t.
> By mistake, I implemented it in the wrong way.
> 
> Also, I did not test it and just compiled it. I apologise for doing so.
> 
> And for the other points you have put as feedback. I will take some time to understand
> it. And would like to work on the changes you suggest.

If you work further on this please make sure that you actually test your
changes. Submitting untested fs changes is a really bad idea. People don't
like it when their data gets corrupted.

Note vboxsf can be tested easily by setting up a VirtualBox guest and then
using the shared folder features. Note do *not* use the VirtualBox provided
guest utils they contain their own out of tree vboxsf implementation and
will use that.

To avoid this you could e.g. use Fedora inside the guest and install
the Fedora packaged vbox guest utils which does not replace the mainline
vboxsf.

Regards,

Hans