| Message ID | 20220831041843.973026-8-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | convert most filesystems to pin_user_pages_fast() | expand |
On Wed, 31 Aug 2022 at 06:19, John Hubbard <jhubbard@nvidia.com> wrote: > > Convert the fuse filesystem to use pin_user_pages_fast() and > unpin_user_page(), instead of get_user_pages_fast() and put_page(). > > The user of pin_user_pages_fast() depends upon: > > 1) CONFIG_BLK_USE_PIN_USER_PAGES_FOR_DIO, and > > 2) User-space-backed pages or ITER_BVEC pages. > > Signed-off-by: John Hubbard <jhubbard@nvidia.com> > --- > fs/fuse/dev.c | 11 +++++++++-- > fs/fuse/file.c | 32 +++++++++++++++++++++----------- > fs/fuse/fuse_i.h | 1 + > 3 files changed, 31 insertions(+), 13 deletions(-) > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > index 51897427a534..5de98a7a45b1 100644 > --- a/fs/fuse/dev.c > +++ b/fs/fuse/dev.c > @@ -675,7 +675,12 @@ static void fuse_copy_finish(struct fuse_copy_state *cs) > flush_dcache_page(cs->pg); > set_page_dirty_lock(cs->pg); > } > - put_page(cs->pg); > + if (!cs->pipebufs && > + (user_backed_iter(cs->iter) || iov_iter_is_bvec(cs->iter))) > + dio_w_unpin_user_page(cs->pg); > + > + else > + put_page(cs->pg); Why not move the logic into a helper and pass a "bool pinned" argument? > } > cs->pg = NULL; > } > @@ -730,7 +735,9 @@ static int fuse_copy_fill(struct fuse_copy_state *cs) > } > } else { > size_t off; > - err = iov_iter_get_pages2(cs->iter, &page, PAGE_SIZE, 1, &off); > + > + err = dio_w_iov_iter_pin_pages(cs->iter, &page, PAGE_SIZE, 1, > + &off); > if (err < 0) > return err; > BUG_ON(!err); > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index 1a3afd469e3a..01da38928d0b 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -625,14 +625,19 @@ void fuse_read_args_fill(struct fuse_io_args *ia, struct file *file, loff_t pos, > } > > static void fuse_release_user_pages(struct fuse_args_pages *ap, > - bool should_dirty) > + bool should_dirty, bool is_user_or_bvec) > { > unsigned int i; > > - for (i = 0; i < ap->num_pages; i++) { > - if (should_dirty) > - set_page_dirty_lock(ap->pages[i]); > - put_page(ap->pages[i]); > + if (is_user_or_bvec) { > + dio_w_unpin_user_pages_dirty_lock(ap->pages, ap->num_pages, > + should_dirty); > + } else { > + for (i = 0; i < ap->num_pages; i++) { > + if (should_dirty) > + set_page_dirty_lock(ap->pages[i]); > + put_page(ap->pages[i]); > + } Same here. > } > } > > @@ -733,7 +738,7 @@ static void fuse_aio_complete_req(struct fuse_mount *fm, struct fuse_args *args, > struct fuse_io_priv *io = ia->io; > ssize_t pos = -1; > > - fuse_release_user_pages(&ia->ap, io->should_dirty); > + fuse_release_user_pages(&ia->ap, io->should_dirty, io->is_user_or_bvec); > > if (err) { > /* Nothing */ > @@ -1414,10 +1419,10 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii, > while (nbytes < *nbytesp && ap->num_pages < max_pages) { > unsigned npages; > size_t start; > - ret = iov_iter_get_pages2(ii, &ap->pages[ap->num_pages], > - *nbytesp - nbytes, > - max_pages - ap->num_pages, > - &start); > + ret = dio_w_iov_iter_pin_pages(ii, &ap->pages[ap->num_pages], > + *nbytesp - nbytes, > + max_pages - ap->num_pages, > + &start); > if (ret < 0) > break; > > @@ -1483,6 +1488,10 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter, > fl_owner_t owner = current->files; > size_t nbytes = min(count, nmax); > > + /* For use in fuse_release_user_pages(): */ > + io->is_user_or_bvec = user_backed_iter(iter) || > + iov_iter_is_bvec(iter); > + How about io->is_pinned? And a iov_iter_is_pinned() helper? Thanks, Miklos
On 8/31/22 03:37, Miklos Szeredi wrote: Hi Miklos, Thanks for looking at this, I'll accept all of these suggestions. > On Wed, 31 Aug 2022 at 06:19, John Hubbard <jhubbard@nvidia.com> wrote: >> >> Convert the fuse filesystem to use pin_user_pages_fast() and >> unpin_user_page(), instead of get_user_pages_fast() and put_page(). >> >> The user of pin_user_pages_fast() depends upon: >> >> 1) CONFIG_BLK_USE_PIN_USER_PAGES_FOR_DIO, and >> >> 2) User-space-backed pages or ITER_BVEC pages. >> >> Signed-off-by: John Hubbard <jhubbard@nvidia.com> >> --- >> fs/fuse/dev.c | 11 +++++++++-- >> fs/fuse/file.c | 32 +++++++++++++++++++++----------- >> fs/fuse/fuse_i.h | 1 + >> 3 files changed, 31 insertions(+), 13 deletions(-) >> >> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c >> index 51897427a534..5de98a7a45b1 100644 >> --- a/fs/fuse/dev.c >> +++ b/fs/fuse/dev.c >> @@ -675,7 +675,12 @@ static void fuse_copy_finish(struct fuse_copy_state *cs) >> flush_dcache_page(cs->pg); >> set_page_dirty_lock(cs->pg); >> } >> - put_page(cs->pg); >> + if (!cs->pipebufs && >> + (user_backed_iter(cs->iter) || iov_iter_is_bvec(cs->iter))) >> + dio_w_unpin_user_page(cs->pg); >> + >> + else >> + put_page(cs->pg); > > Why not move the logic into a helper and pass a "bool pinned" argument? OK, will do. It's not yet clear from the discussion in the other thread with Jan and Al [1], if I'll end up keeping this check: user_backed_iter(cs->iter) || iov_iter_is_bvec(cs->iter) ...but if it stays, then the helper is a good idea. > >> } >> cs->pg = NULL; >> } >> @@ -730,7 +735,9 @@ static int fuse_copy_fill(struct fuse_copy_state *cs) >> } >> } else { >> size_t off; >> - err = iov_iter_get_pages2(cs->iter, &page, PAGE_SIZE, 1, &off); >> + >> + err = dio_w_iov_iter_pin_pages(cs->iter, &page, PAGE_SIZE, 1, >> + &off); >> if (err < 0) >> return err; >> BUG_ON(!err); >> diff --git a/fs/fuse/file.c b/fs/fuse/file.c >> index 1a3afd469e3a..01da38928d0b 100644 >> --- a/fs/fuse/file.c >> +++ b/fs/fuse/file.c >> @@ -625,14 +625,19 @@ void fuse_read_args_fill(struct fuse_io_args *ia, struct file *file, loff_t pos, >> } >> >> static void fuse_release_user_pages(struct fuse_args_pages *ap, >> - bool should_dirty) >> + bool should_dirty, bool is_user_or_bvec) >> { >> unsigned int i; >> >> - for (i = 0; i < ap->num_pages; i++) { >> - if (should_dirty) >> - set_page_dirty_lock(ap->pages[i]); >> - put_page(ap->pages[i]); >> + if (is_user_or_bvec) { >> + dio_w_unpin_user_pages_dirty_lock(ap->pages, ap->num_pages, >> + should_dirty); >> + } else { >> + for (i = 0; i < ap->num_pages; i++) { >> + if (should_dirty) >> + set_page_dirty_lock(ap->pages[i]); >> + put_page(ap->pages[i]); >> + } > > Same here. Yes. Definitely belongs in a helper function. I was thinking, "don't go that far, because the code will eventually get deleted anyway", but you are right. :) > >> } >> } >> >> @@ -733,7 +738,7 @@ static void fuse_aio_complete_req(struct fuse_mount *fm, struct fuse_args *args, >> struct fuse_io_priv *io = ia->io; >> ssize_t pos = -1; >> >> - fuse_release_user_pages(&ia->ap, io->should_dirty); >> + fuse_release_user_pages(&ia->ap, io->should_dirty, io->is_user_or_bvec); >> >> if (err) { >> /* Nothing */ >> @@ -1414,10 +1419,10 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii, >> while (nbytes < *nbytesp && ap->num_pages < max_pages) { >> unsigned npages; >> size_t start; >> - ret = iov_iter_get_pages2(ii, &ap->pages[ap->num_pages], >> - *nbytesp - nbytes, >> - max_pages - ap->num_pages, >> - &start); >> + ret = dio_w_iov_iter_pin_pages(ii, &ap->pages[ap->num_pages], >> + *nbytesp - nbytes, >> + max_pages - ap->num_pages, >> + &start); >> if (ret < 0) >> break; >> >> @@ -1483,6 +1488,10 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter, >> fl_owner_t owner = current->files; >> size_t nbytes = min(count, nmax); >> >> + /* For use in fuse_release_user_pages(): */ >> + io->is_user_or_bvec = user_backed_iter(iter) || >> + iov_iter_is_bvec(iter); >> + > > How about io->is_pinned? And a iov_iter_is_pinned() helper? Agreed, is_pinned is a better name, and the helper (if we end up needing that logic) also sounds good. [1] https://lore.kernel.org/r/20220831094349.boln4jjajkdtykx3@quack3 thanks,
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index 51897427a534..5de98a7a45b1 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -675,7 +675,12 @@ static void fuse_copy_finish(struct fuse_copy_state *cs) flush_dcache_page(cs->pg); set_page_dirty_lock(cs->pg); } - put_page(cs->pg); + if (!cs->pipebufs && + (user_backed_iter(cs->iter) || iov_iter_is_bvec(cs->iter))) + dio_w_unpin_user_page(cs->pg); + + else + put_page(cs->pg); } cs->pg = NULL; } @@ -730,7 +735,9 @@ static int fuse_copy_fill(struct fuse_copy_state *cs) } } else { size_t off; - err = iov_iter_get_pages2(cs->iter, &page, PAGE_SIZE, 1, &off); + + err = dio_w_iov_iter_pin_pages(cs->iter, &page, PAGE_SIZE, 1, + &off); if (err < 0) return err; BUG_ON(!err); diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 1a3afd469e3a..01da38928d0b 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -625,14 +625,19 @@ void fuse_read_args_fill(struct fuse_io_args *ia, struct file *file, loff_t pos, } static void fuse_release_user_pages(struct fuse_args_pages *ap, - bool should_dirty) + bool should_dirty, bool is_user_or_bvec) { unsigned int i; - for (i = 0; i < ap->num_pages; i++) { - if (should_dirty) - set_page_dirty_lock(ap->pages[i]); - put_page(ap->pages[i]); + if (is_user_or_bvec) { + dio_w_unpin_user_pages_dirty_lock(ap->pages, ap->num_pages, + should_dirty); + } else { + for (i = 0; i < ap->num_pages; i++) { + if (should_dirty) + set_page_dirty_lock(ap->pages[i]); + put_page(ap->pages[i]); + } } } @@ -733,7 +738,7 @@ static void fuse_aio_complete_req(struct fuse_mount *fm, struct fuse_args *args, struct fuse_io_priv *io = ia->io; ssize_t pos = -1; - fuse_release_user_pages(&ia->ap, io->should_dirty); + fuse_release_user_pages(&ia->ap, io->should_dirty, io->is_user_or_bvec); if (err) { /* Nothing */ @@ -1414,10 +1419,10 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii, while (nbytes < *nbytesp && ap->num_pages < max_pages) { unsigned npages; size_t start; - ret = iov_iter_get_pages2(ii, &ap->pages[ap->num_pages], - *nbytesp - nbytes, - max_pages - ap->num_pages, - &start); + ret = dio_w_iov_iter_pin_pages(ii, &ap->pages[ap->num_pages], + *nbytesp - nbytes, + max_pages - ap->num_pages, + &start); if (ret < 0) break; @@ -1483,6 +1488,10 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter, fl_owner_t owner = current->files; size_t nbytes = min(count, nmax); + /* For use in fuse_release_user_pages(): */ + io->is_user_or_bvec = user_backed_iter(iter) || + iov_iter_is_bvec(iter); + err = fuse_get_user_pages(&ia->ap, iter, &nbytes, write, max_pages); if (err && !nbytes) @@ -1498,7 +1507,8 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter, } if (!io->async || nres < 0) { - fuse_release_user_pages(&ia->ap, io->should_dirty); + fuse_release_user_pages(&ia->ap, io->should_dirty, + io->is_user_or_bvec); fuse_io_free(ia); } ia = NULL; diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 488b460e046f..6ee7f72e29eb 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -290,6 +290,7 @@ struct fuse_io_priv { struct kiocb *iocb; struct completion *done; bool blocking; + bool is_user_or_bvec; }; #define FUSE_IO_PRIV_SYNC(i) \
Convert the fuse filesystem to use pin_user_pages_fast() and unpin_user_page(), instead of get_user_pages_fast() and put_page(). The user of pin_user_pages_fast() depends upon: 1) CONFIG_BLK_USE_PIN_USER_PAGES_FOR_DIO, and 2) User-space-backed pages or ITER_BVEC pages. Signed-off-by: John Hubbard <jhubbard@nvidia.com> --- fs/fuse/dev.c | 11 +++++++++-- fs/fuse/file.c | 32 +++++++++++++++++++++----------- fs/fuse/fuse_i.h | 1 + 3 files changed, 31 insertions(+), 13 deletions(-)