| Message ID | 20180604193123.27655-7-agruenba@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2018-06-04 21:31 GMT+02:00 Andreas Gruenbacher <agruenba@redhat.com>: > Add a page_write_end hook called when done writing to a page, for > filesystems that implement data journaling: in that case, pages are > written to the journal before being written back to their proper on-disk > locations. > The new hook is bypassed for IOMAP_INLINE mappings. Oops, no longer true of course. > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> > --- > fs/iomap.c | 58 ++++++++++++++++++++++++++++++++----------- > include/linux/iomap.h | 8 ++++++ > 2 files changed, 52 insertions(+), 14 deletions(-) > > diff --git a/fs/iomap.c b/fs/iomap.c > index 48cd67227811..3b34c957d2fd 100644 > --- a/fs/iomap.c > +++ b/fs/iomap.c > @@ -181,16 +181,22 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags, > > static int > iomap_write_end(struct inode *inode, loff_t pos, unsigned len, > - unsigned copied, struct page *page, struct iomap *iomap) > + unsigned copied, struct page *page, struct iomap *iomap, > + const struct iomap_ops *ops) > { > + typeof(ops->page_write_end) page_write_end = ops->page_write_end; > int ret; > > if (iomap->type == IOMAP_INLINE) { > iomap_write_inline_data(inode, page, iomap, pos, copied); > __generic_write_end(inode, pos, copied, page); > + if (page_write_end) > + page_write_end(inode, pos, copied, page, iomap); > return copied; > } > > + if (page_write_end) > + page_write_end(inode, pos, copied, page, iomap); > ret = generic_write_end(NULL, inode->i_mapping, pos, len, > copied, page, NULL); > if (ret < len) > @@ -198,11 +204,17 @@ iomap_write_end(struct inode *inode, loff_t pos, unsigned len, > return ret; > } > > +struct iomap_write_args { > + const struct iomap_ops *ops; > + struct iov_iter *iter; > +}; > + > static loff_t > iomap_write_actor(struct inode *inode, loff_t pos, loff_t length, void *data, > struct iomap *iomap) > { > - struct iov_iter *i = data; > + struct iomap_write_args *args = data; > + struct iov_iter *i = args->iter; > long status = 0; > ssize_t written = 0; > unsigned int flags = AOP_FLAG_NOFS; > @@ -248,7 +260,7 @@ iomap_write_actor(struct inode *inode, loff_t pos, loff_t length, void *data, > flush_dcache_page(page); > > status = iomap_write_end(inode, pos, bytes, copied, page, > - iomap); > + iomap, args->ops); > if (unlikely(status < 0)) > break; > copied = status; > @@ -285,10 +297,14 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *iter, > { > struct inode *inode = iocb->ki_filp->f_mapping->host; > loff_t pos = iocb->ki_pos, ret = 0, written = 0; > + struct iomap_write_args args = { > + .ops = ops, > + .iter = iter, > + }; > > while (iov_iter_count(iter)) { > ret = iomap_apply(inode, pos, iov_iter_count(iter), > - IOMAP_WRITE, ops, iter, iomap_write_actor); > + IOMAP_WRITE, ops, &args, iomap_write_actor); > if (ret <= 0) > break; > pos += ret; > @@ -319,6 +335,7 @@ static loff_t > iomap_dirty_actor(struct inode *inode, loff_t pos, loff_t length, void *data, > struct iomap *iomap) > { > + const struct iomap_ops *ops = data; > long status = 0; > ssize_t written = 0; > > @@ -342,7 +359,8 @@ iomap_dirty_actor(struct inode *inode, loff_t pos, loff_t length, void *data, > > WARN_ON_ONCE(!PageUptodate(page)); > > - status = iomap_write_end(inode, pos, bytes, bytes, page, iomap); > + status = iomap_write_end(inode, pos, bytes, bytes, page, iomap, > + ops); > if (unlikely(status <= 0)) { > if (WARN_ON_ONCE(status == 0)) > return -EIO; > @@ -368,8 +386,8 @@ iomap_file_dirty(struct inode *inode, loff_t pos, loff_t len, > loff_t ret; > > while (len) { > - ret = iomap_apply(inode, pos, len, IOMAP_WRITE, ops, NULL, > - iomap_dirty_actor); > + ret = iomap_apply(inode, pos, len, IOMAP_WRITE, ops, > + (void *)ops, iomap_dirty_actor); > if (ret <= 0) > return ret; > pos += ret; > @@ -381,7 +399,8 @@ iomap_file_dirty(struct inode *inode, loff_t pos, loff_t len, > EXPORT_SYMBOL_GPL(iomap_file_dirty); > > static int iomap_zero(struct inode *inode, loff_t pos, unsigned offset, > - unsigned bytes, struct iomap *iomap) > + unsigned bytes, const struct iomap_ops *ops, > + struct iomap *iomap) > { > struct page *page; > int status; > @@ -394,7 +413,8 @@ static int iomap_zero(struct inode *inode, loff_t pos, unsigned offset, > zero_user(page, offset, bytes); > mark_page_accessed(page); > > - return iomap_write_end(inode, pos, bytes, bytes, page, iomap); > + return iomap_write_end(inode, pos, bytes, bytes, page, iomap, > + ops); > } > > static int iomap_dax_zero(loff_t pos, unsigned offset, unsigned bytes, > @@ -407,11 +427,16 @@ static int iomap_dax_zero(loff_t pos, unsigned offset, unsigned bytes, > offset, bytes); > } > > +struct iomap_zero_range_args { > + const struct iomap_ops *ops; > + bool *did_zero; > +}; > + > static loff_t > iomap_zero_range_actor(struct inode *inode, loff_t pos, loff_t count, > void *data, struct iomap *iomap) > { > - bool *did_zero = data; > + struct iomap_zero_range_args *args = data; > loff_t written = 0; > int status; > > @@ -428,15 +453,16 @@ iomap_zero_range_actor(struct inode *inode, loff_t pos, loff_t count, > if (IS_DAX(inode)) > status = iomap_dax_zero(pos, offset, bytes, iomap); > else > - status = iomap_zero(inode, pos, offset, bytes, iomap); > + status = iomap_zero(inode, pos, offset, bytes, > + args->ops, iomap); > if (status < 0) > return status; > > pos += bytes; > count -= bytes; > written += bytes; > - if (did_zero) > - *did_zero = true; > + if (args->did_zero) > + *args->did_zero = true; > } while (count > 0); > > return written; > @@ -446,11 +472,15 @@ int > iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero, > const struct iomap_ops *ops) > { > + struct iomap_zero_range_args args = { > + .ops = ops, > + .did_zero = did_zero, > + }; > loff_t ret; > > while (len > 0) { > ret = iomap_apply(inode, pos, len, IOMAP_ZERO, > - ops, did_zero, iomap_zero_range_actor); > + ops, &args, iomap_zero_range_actor); > if (ret <= 0) > return ret; > > diff --git a/include/linux/iomap.h b/include/linux/iomap.h > index c61113c71a60..ac7f1b2c1cbe 100644 > --- a/include/linux/iomap.h > +++ b/include/linux/iomap.h > @@ -8,6 +8,7 @@ struct fiemap_extent_info; > struct inode; > struct iov_iter; > struct kiocb; > +struct page; > struct vm_area_struct; > struct vm_fault; > > @@ -71,6 +72,13 @@ struct iomap_ops { > int (*iomap_begin)(struct inode *inode, loff_t pos, loff_t length, > unsigned flags, struct iomap *iomap); > > + /* > + * Called when done writing to a page (optional; skipped for > + * IOMAP_INLINE mappings). The comment needs updating too, of course. > + */ > + void (*page_write_end)(struct inode *inode, loff_t pos, unsigned copied, > + struct page *page, struct iomap *iomap); > + > /* > * Commit and/or unreserve space previous allocated using iomap_begin. > * Written indicates the length of the successful write operation which > -- > 2.17.0 > With those two changes, is this commit okay as well? Thanks, Andreas
On Mon, Jun 04, 2018 at 09:31:19PM +0200, Andreas Gruenbacher wrote: > --- a/fs/iomap.c > +++ b/fs/iomap.c > @@ -181,16 +181,22 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags, > > static int > iomap_write_end(struct inode *inode, loff_t pos, unsigned len, > - unsigned copied, struct page *page, struct iomap *iomap) > + unsigned copied, struct page *page, struct iomap *iomap, > + const struct iomap_ops *ops) > { > + typeof(ops->page_write_end) page_write_end = ops->page_write_end; Is the reason to use typeof is to avoid repeating the type of page_write_end? As it's only for a temporary variable with 2 uses, ops->page_write_end does not hurt readability nor is too much typing. I would not recommend using typeof outside of the justified contexts like macros or without a good reason. > int ret; > > if (iomap->type == IOMAP_INLINE) { > iomap_write_inline_data(inode, page, iomap, pos, copied); > __generic_write_end(inode, pos, copied, page); > + if (page_write_end) > + page_write_end(inode, pos, copied, page, iomap); > return copied; > } > > + if (page_write_end) > + page_write_end(inode, pos, copied, page, iomap); > ret = generic_write_end(NULL, inode->i_mapping, pos, len, > copied, page, NULL); > if (ret < len) > @@ -198,11 +204,17 @@ iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
2018-06-05 14:07 GMT+02:00 David Sterba <dsterba@suse.cz>: > On Mon, Jun 04, 2018 at 09:31:19PM +0200, Andreas Gruenbacher wrote: >> --- a/fs/iomap.c >> +++ b/fs/iomap.c >> @@ -181,16 +181,22 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags, >> >> static int >> iomap_write_end(struct inode *inode, loff_t pos, unsigned len, >> - unsigned copied, struct page *page, struct iomap *iomap) >> + unsigned copied, struct page *page, struct iomap *iomap, >> + const struct iomap_ops *ops) >> { >> + typeof(ops->page_write_end) page_write_end = ops->page_write_end; > > Is the reason to use typeof is to avoid repeating the type of > page_write_end? Yes, the type is void (*)(struct inode *, loff_t, unsigned, struct page *, struct iomap *), which is a bit bulky. > As it's only for a temporary variable with 2 uses, > ops->page_write_end does not hurt readability nor is too much typing. > I would not recommend using typeof outside of the justified contexts > like macros or without a good reason. Andreas
On Tue, Jun 05, 2018 at 02:17:38PM +0200, Andreas Grünbacher wrote: > 2018-06-05 14:07 GMT+02:00 David Sterba <dsterba@suse.cz>: > > On Mon, Jun 04, 2018 at 09:31:19PM +0200, Andreas Gruenbacher wrote: > >> --- a/fs/iomap.c > >> +++ b/fs/iomap.c > >> @@ -181,16 +181,22 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags, > >> > >> static int > >> iomap_write_end(struct inode *inode, loff_t pos, unsigned len, > >> - unsigned copied, struct page *page, struct iomap *iomap) > >> + unsigned copied, struct page *page, struct iomap *iomap, > >> + const struct iomap_ops *ops) > >> { > >> + typeof(ops->page_write_end) page_write_end = ops->page_write_end; > > > > Is the reason to use typeof is to avoid repeating the type of > > page_write_end? > > Yes, the type is void (*)(struct inode *, loff_t, unsigned, struct > page *, struct iomap *), which is a bit bulky. Agreed, so why don't you simply use ops->page_write_end ?
2018-06-05 14:29 GMT+02:00 David Sterba <dsterba@suse.cz>: > On Tue, Jun 05, 2018 at 02:17:38PM +0200, Andreas Grünbacher wrote: >> 2018-06-05 14:07 GMT+02:00 David Sterba <dsterba@suse.cz>: >> > On Mon, Jun 04, 2018 at 09:31:19PM +0200, Andreas Gruenbacher wrote: >> >> --- a/fs/iomap.c >> >> +++ b/fs/iomap.c >> >> @@ -181,16 +181,22 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags, >> >> >> >> static int >> >> iomap_write_end(struct inode *inode, loff_t pos, unsigned len, >> >> - unsigned copied, struct page *page, struct iomap *iomap) >> >> + unsigned copied, struct page *page, struct iomap *iomap, >> >> + const struct iomap_ops *ops) >> >> { >> >> + typeof(ops->page_write_end) page_write_end = ops->page_write_end; >> > >> > Is the reason to use typeof is to avoid repeating the type of >> > page_write_end? >> >> Yes, the type is void (*)(struct inode *, loff_t, unsigned, struct >> page *, struct iomap *), which is a bit bulky. > > Agreed, so why don't you simply use ops->page_write_end ? Alright. Thanks, Andreas
diff --git a/fs/iomap.c b/fs/iomap.c index 48cd67227811..3b34c957d2fd 100644 --- a/fs/iomap.c +++ b/fs/iomap.c @@ -181,16 +181,22 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags, static int iomap_write_end(struct inode *inode, loff_t pos, unsigned len, - unsigned copied, struct page *page, struct iomap *iomap) + unsigned copied, struct page *page, struct iomap *iomap, + const struct iomap_ops *ops) { + typeof(ops->page_write_end) page_write_end = ops->page_write_end; int ret; if (iomap->type == IOMAP_INLINE) { iomap_write_inline_data(inode, page, iomap, pos, copied); __generic_write_end(inode, pos, copied, page); + if (page_write_end) + page_write_end(inode, pos, copied, page, iomap); return copied; } + if (page_write_end) + page_write_end(inode, pos, copied, page, iomap); ret = generic_write_end(NULL, inode->i_mapping, pos, len, copied, page, NULL); if (ret < len) @@ -198,11 +204,17 @@ iomap_write_end(struct inode *inode, loff_t pos, unsigned len, return ret; } +struct iomap_write_args { + const struct iomap_ops *ops; + struct iov_iter *iter; +}; + static loff_t iomap_write_actor(struct inode *inode, loff_t pos, loff_t length, void *data, struct iomap *iomap) { - struct iov_iter *i = data; + struct iomap_write_args *args = data; + struct iov_iter *i = args->iter; long status = 0; ssize_t written = 0; unsigned int flags = AOP_FLAG_NOFS; @@ -248,7 +260,7 @@ iomap_write_actor(struct inode *inode, loff_t pos, loff_t length, void *data, flush_dcache_page(page); status = iomap_write_end(inode, pos, bytes, copied, page, - iomap); + iomap, args->ops); if (unlikely(status < 0)) break; copied = status; @@ -285,10 +297,14 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *iter, { struct inode *inode = iocb->ki_filp->f_mapping->host; loff_t pos = iocb->ki_pos, ret = 0, written = 0; + struct iomap_write_args args = { + .ops = ops, + .iter = iter, + }; while (iov_iter_count(iter)) { ret = iomap_apply(inode, pos, iov_iter_count(iter), - IOMAP_WRITE, ops, iter, iomap_write_actor); + IOMAP_WRITE, ops, &args, iomap_write_actor); if (ret <= 0) break; pos += ret; @@ -319,6 +335,7 @@ static loff_t iomap_dirty_actor(struct inode *inode, loff_t pos, loff_t length, void *data, struct iomap *iomap) { + const struct iomap_ops *ops = data; long status = 0; ssize_t written = 0; @@ -342,7 +359,8 @@ iomap_dirty_actor(struct inode *inode, loff_t pos, loff_t length, void *data, WARN_ON_ONCE(!PageUptodate(page)); - status = iomap_write_end(inode, pos, bytes, bytes, page, iomap); + status = iomap_write_end(inode, pos, bytes, bytes, page, iomap, + ops); if (unlikely(status <= 0)) { if (WARN_ON_ONCE(status == 0)) return -EIO; @@ -368,8 +386,8 @@ iomap_file_dirty(struct inode *inode, loff_t pos, loff_t len, loff_t ret; while (len) { - ret = iomap_apply(inode, pos, len, IOMAP_WRITE, ops, NULL, - iomap_dirty_actor); + ret = iomap_apply(inode, pos, len, IOMAP_WRITE, ops, + (void *)ops, iomap_dirty_actor); if (ret <= 0) return ret; pos += ret; @@ -381,7 +399,8 @@ iomap_file_dirty(struct inode *inode, loff_t pos, loff_t len, EXPORT_SYMBOL_GPL(iomap_file_dirty); static int iomap_zero(struct inode *inode, loff_t pos, unsigned offset, - unsigned bytes, struct iomap *iomap) + unsigned bytes, const struct iomap_ops *ops, + struct iomap *iomap) { struct page *page; int status; @@ -394,7 +413,8 @@ static int iomap_zero(struct inode *inode, loff_t pos, unsigned offset, zero_user(page, offset, bytes); mark_page_accessed(page); - return iomap_write_end(inode, pos, bytes, bytes, page, iomap); + return iomap_write_end(inode, pos, bytes, bytes, page, iomap, + ops); } static int iomap_dax_zero(loff_t pos, unsigned offset, unsigned bytes, @@ -407,11 +427,16 @@ static int iomap_dax_zero(loff_t pos, unsigned offset, unsigned bytes, offset, bytes); } +struct iomap_zero_range_args { + const struct iomap_ops *ops; + bool *did_zero; +}; + static loff_t iomap_zero_range_actor(struct inode *inode, loff_t pos, loff_t count, void *data, struct iomap *iomap) { - bool *did_zero = data; + struct iomap_zero_range_args *args = data; loff_t written = 0; int status; @@ -428,15 +453,16 @@ iomap_zero_range_actor(struct inode *inode, loff_t pos, loff_t count, if (IS_DAX(inode)) status = iomap_dax_zero(pos, offset, bytes, iomap); else - status = iomap_zero(inode, pos, offset, bytes, iomap); + status = iomap_zero(inode, pos, offset, bytes, + args->ops, iomap); if (status < 0) return status; pos += bytes; count -= bytes; written += bytes; - if (did_zero) - *did_zero = true; + if (args->did_zero) + *args->did_zero = true; } while (count > 0); return written; @@ -446,11 +472,15 @@ int iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero, const struct iomap_ops *ops) { + struct iomap_zero_range_args args = { + .ops = ops, + .did_zero = did_zero, + }; loff_t ret; while (len > 0) { ret = iomap_apply(inode, pos, len, IOMAP_ZERO, - ops, did_zero, iomap_zero_range_actor); + ops, &args, iomap_zero_range_actor); if (ret <= 0) return ret; diff --git a/include/linux/iomap.h b/include/linux/iomap.h index c61113c71a60..ac7f1b2c1cbe 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -8,6 +8,7 @@ struct fiemap_extent_info; struct inode; struct iov_iter; struct kiocb; +struct page; struct vm_area_struct; struct vm_fault; @@ -71,6 +72,13 @@ struct iomap_ops { int (*iomap_begin)(struct inode *inode, loff_t pos, loff_t length, unsigned flags, struct iomap *iomap); + /* + * Called when done writing to a page (optional; skipped for + * IOMAP_INLINE mappings). + */ + void (*page_write_end)(struct inode *inode, loff_t pos, unsigned copied, + struct page *page, struct iomap *iomap); + /* * Commit and/or unreserve space previous allocated using iomap_begin. * Written indicates the length of the successful write operation which
Add a page_write_end hook called when done writing to a page, for filesystems that implement data journaling: in that case, pages are written to the journal before being written back to their proper on-disk locations. The new hook is bypassed for IOMAP_INLINE mappings. Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com> --- fs/iomap.c | 58 ++++++++++++++++++++++++++++++++----------- include/linux/iomap.h | 8 ++++++ 2 files changed, 52 insertions(+), 14 deletions(-)