On Wed, 18 Oct 2023 at 14:17, Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> 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?
The difficulty we have with kmap_local_*() has to do with the
requirement that maps and unmaps need to be nested neatly. For
example:
let a = folio1.map_local(...);
let b = folio2.map_local(...);
// Do something with `a` and `b`.
drop(a);
drop(b);
The code obviously violates the requirements.
One way to enforce the rule is Rust is to use closures, so the code
above would be:
folio1.map_local(..., |a| {
folio2.map_local(..., |b| {
// Do something with `a` and `b`.
})
})
It isn't ergonomic the first option, but allows us to satisfy the
nesting requirement.
Any chance we can relax that requirement?
(If not, and we really want to get rid of the non-local function, we
can fall back to the closure-based implementation. In fact, you'll
find that in this patch I already do this for a private function that
used when writing into the folio, we could just make a version of it
public.)
> > +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?
I'm just mimicking C code. Happy to remove it.
> > + /// 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).
I'm also just mimicking C here -- we just don't have a type that has
the properties you describe. I'm happy to switch once we have it, in
fact, Miguel has plans that I believe align well with what you want.
I'm not sure if he has already contacted you about it yet though.
> > +/// 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?
Instances of `UniqueFolio` are only produced by calls to
`folio_alloc`. They encode the fact that it's safe for us to map the
folio and know that there aren't any concurrent threads/CPUs doing the
same to the same folio.
Naturally, if you to increment the refcount on this folio and share it
with other threads/CPUs, it's no longer unique. So we don't allow it.
This is only used when using a synchronous bio to read blocks from a
block device while setting up a new superblock, in particular, to read
the superblock itself.
>
> > +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) };
The problem is the unmap path that can happen at arbitrary order in
Rust, see my comment above.
>
> > + // 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?
Do you have ideas on how this might look like? (Don't worry about
Rust, just express it in some pseudo-C and we'll see if you can
express it in Rust.)
My approach here was to be conservative, since the common denominator
was "maps to the end of the page", that's what I have.
One possible way to do it with Rust would be an "Iterator" -- in
HIGHMEM it would return one item per page, otherwise it would return a
single item. (We have something similar for reading arbitrary ranges
of a block device, it's broken up into several chunks, so we return an
iterator.)
