Re: [RFC PATCH 09/19] rust: folio: introduce basic support for folios

[Matthew Wilcox <willy@infradead.org>]

On Wed, Oct 18, 2023 at 09:25:08AM -0300, Wedson Almeida Filho wrote:
> +void *rust_helper_kmap(struct page *page)
> +{
> +	return kmap(page);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_kmap);
> +
> +void rust_helper_kunmap(struct page *page)
> +{
> +	kunmap(page);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_kunmap);

I'm not thrilled by exposing kmap()/kunmap() to Rust code.  The vast
majority of code really only needs kmap_local_*() / kunmap_local().
Can you elaborate on why you need the old kmap() in new Rust code?

> +void rust_helper_folio_set_error(struct folio *folio)
> +{
> +	folio_set_error(folio);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_folio_set_error);

I'm trying to get rid of the error flag.  Can you share the situations
in which you've needed the error flag?  Or is it just copying existing
practices?

> +    /// Returns the byte position of this folio in its file.
> +    pub fn pos(&self) -> i64 {
> +        // SAFETY: The folio is valid because the shared reference implies a non-zero refcount.
> +        unsafe { bindings::folio_pos(self.0.get()) }
> +    }

I think it's a mistake to make file positions an i64.  I estimate 64
bits will not be enough by 2035-2040.  We should probably have a numeric
type which is i64 on 32-bit and isize on other CPUs (I also project
64-bit pointers will be insufficient by 2035-2040 and so we will have
128-bit pointers around the same time, so we're not going to need i128
file offsets with i64 pointers).

> +/// A [`Folio`] that has a single reference to it.
> +pub struct UniqueFolio(pub(crate) ARef<Folio>);

How do we know it only has a single reference?  Do you mean "has at
least one reference"?  Or am I confusing Rust's notion of a reference
with Linux's notion of a reference?

> +impl UniqueFolio {
> +    /// Maps the contents of a folio page into a slice.
> +    pub fn map_page(&self, page_index: usize) -> Result<MapGuard<'_>> {
> +        if page_index >= self.0.size() / bindings::PAGE_SIZE {
> +            return Err(EDOM);
> +        }
> +
> +        // SAFETY: We just checked that the index is within bounds of the folio.
> +        let page = unsafe { bindings::folio_page(self.0 .0.get(), page_index) };
> +
> +        // SAFETY: `page` is valid because it was returned by `folio_page` above.
> +        let ptr = unsafe { bindings::kmap(page) };

Surely this can be:

	   let ptr = unsafe { bindings::kmap_local_folio(folio, page_index * PAGE_SIZE) };

> +        // SAFETY: We just mapped `ptr`, so it's valid for read.
> +        let data = unsafe { core::slice::from_raw_parts(ptr.cast::<u8>(), bindings::PAGE_SIZE) };

Can we hide away the "if this isn't a HIGHMEM system, this maps to the
end of the folio, but if it is, it only maps to the end of the page"
problem here?