On Wed, 18 Oct 2023 at 14:20, Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> On Wed, Oct 18, 2023 at 02:05:51PM -0300, Wedson Almeida Filho wrote:
> > On Wed, 18 Oct 2023 at 13:57, Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, Oct 18, 2023 at 09:25:18AM -0300, Wedson Almeida Filho wrote:
> > > > + fn read_folio(inode: &INode<Self>, mut folio: LockedFolio<'_>) -> Result {
> > > > + let pos = u64::try_from(folio.pos()).unwrap_or(u64::MAX);
> > > > + let size = u64::try_from(inode.size())?;
> > > > + let sb = inode.super_block();
> > > > +
> > > > + let copied = if pos >= size {
> > > > + 0
> > > > + } else {
> > > > + let offset = inode.data().offset.checked_add(pos).ok_or(ERANGE)?;
> > > > + let len = core::cmp::min(size - pos, folio.size().try_into()?);
> > > > + let mut foffset = 0;
> > > > +
> > > > + if offset.checked_add(len).ok_or(ERANGE)? > sb.data().data_size {
> > > > + return Err(EIO);
> > > > + }
> > > > +
> > > > + for v in sb.read(offset, len)? {
> > > > + let v = v?;
> > > > + folio.write(foffset, v.data())?;
> > > > + foffset += v.data().len();
> > > > + }
> > > > + foffset
> > > > + };
> > > > +
> > > > + folio.zero_out(copied, folio.size() - copied)?;
> > > > + folio.mark_uptodate();
> > > > + folio.flush_dcache();
> > > > +
> > > > + Ok(())
> > > > + }
> > >
> > > Who unlocks the folio here?
> >
> > The `Drop` implementation of `LockedFolio`.
> >
> > Note that `read_folio` is given ownership of `folio` (the last
> > argument), so when it goes out of scope (or when it's explicitly
> > dropped) its `drop` function is called automatically. You'll its
> > implementation (and the call to `folio_unlock`) in patch 9.
>
> That works for synchronous implementations of read_folio(), but for
> an asynchronous implementation, we need to unlock the folio once the
> read completes, typically in the bio completion handler. What's the
> plan for that? Hand ownership of the folio to the bio submission path,
> which hands it to the bio completion path, which drops the folio?
Yes, exactly. (I mentioned this in a github comment a few days back:
https://github.com/Rust-for-Linux/linux/pull/1037#discussion_r1359706872.)
The code as is doesn't allow a `LockedFolio` to outlive this call but
we can add support as you described.
Part of the reason support for this isn't included in this series is
that are no Rust users of the async code path. And we've been told
repeatedly by Greg and others that we must not add code that isn't
used yet.
