On Mon, Sep 15, 2025 at 4:01 AM Nikita Kalyazin <[email protected]> wrote: > > On 13/09/2025 01:32, Vishal Annapurve wrote: > > On Fri, Sep 12, 2025 at 3:35 PM James Houghton <[email protected]> > > wrote: > >> > >>>>> + > >>>>> + if (folio_test_uptodate(folio)) { > >>>>> + folio_unlock(folio); > >>>>> + folio_put(folio); > >>>>> + return -ENOSPC; > >>>> > >>>> Does it actually matter for the folio not to be uptodate? It seems > >>>> unnecessarily restrictive not to be able to overwrite data if we're > >>>> saying that this is only usable for unencrypted memory anyway. > >>> > >>> In the context of direct map removal [1] it does actually because when > >>> we mark a folio as prepared, we remove it from the direct map making it > >>> inaccessible to the way write() performs the copy. It does not matter > >>> if direct map removal isn't enabled though. Do you think it should be > >>> conditional? > >> > >> Oh, good point. It's simpler (both to implement and to describe) to > >> disallow a second write() call in all cases (no matter if the direct > >> map for the page has been removed or if the contents have been > >> encrypted), so I'm all for leaving it unconditional like you have now. > >> Thanks! > > > > Are we deviating from the way read/write semantics work for the other > > filesystems? I don't think other filesystems carry this restriction of > > one-time-write only. Do we strictly need the differing semantics? > > Yes, I believe we are deviating from other "regular" filesystems, but I > don't think what we propose is too uncommon as in "special" filesystems > such as sysfs subsequent calls to attributes like "remove" will likely > fail as well (not due to up-to-date flag though). > > > Maybe it would be simpler to not overload uptodate flag and just not > > allow read/write if folio is not mapped in the direct map for non-conf > > VMs (assuming there could be other ways to deduce that information). > > The only such interface I'm aware of is kernel_page_present() so the > check may look like: > > for (i = 0; i < folio_nr_pages(folio); i++) { > struct page *page = folio_page(folio, i); > if (!kernel_page_present(page)) { > folio_unlock(folio); > folio_put(folio); > return -ENOSPC; > } > } > > However, kernel_page_present() is not currently exported to modules.
I think it should be exposed if there is no cleaner way to deduce if a folio is mapped in the direct map. That being said, we should probably cleanly separate the series to add write population support and the series for removal from direct map [1] and figure out the order in which they need to be merged upstream. I would still vote for not overloading folio_test_uptodate() in either series. Ackerley and Fuad are planning to post a series just for supporting in-place conversion for 4K pages which is going to introduce a maple tree for storing private/shared-ness of ranges. We could possibly augment the support to track directmap removal there. RFC version [2] is a good reference for now. [1] https://lore.kernel.org/kvm/[email protected]/ [2] https://lore.kernel.org/kvm/d3832fd95a03aad562705872cbda5b3d248ca321.1747264138.git.ackerley...@google.com/#t
