| Message ID | 20220518233709.1937634-16-shr@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | io-uring/xfs: support async buffered writes | expand |
> +static int balance_dirty_pages_ratelimited_flags(struct address_space *mapping, > + bool no_wait) > { This doesn't actully take flags, but a single boolean argument. So either it needs a new name, or we actually pass a descriptiv flag. > +/** > + * balance_dirty_pages_ratelimited_async - balance dirty memory state > + * @mapping: address_space which was dirtied > + * > + * Processes which are dirtying memory should call in here once for each page > + * which was newly dirtied. The function will periodically check the system's > + * dirty state and will initiate writeback if needed. > + * > + * Once we're over the dirty memory limit we decrease the ratelimiting > + * by a lot, to prevent individual processes from overshooting the limit > + * by (ratelimit_pages) each. > + * > + * This is the async version of the API. It only checks if it is required to > + * balance dirty pages. In case it needs to balance dirty pages, it returns > + * -EAGAIN. > + */ > +int balance_dirty_pages_ratelimited_async(struct address_space *mapping) > +{ > + return balance_dirty_pages_ratelimited_flags(mapping, true); > +} > +EXPORT_SYMBOL(balance_dirty_pages_ratelimited_async); I'd much rather export the underlying balance_dirty_pages_ratelimited_flags helper than adding a pointless wrapper here. And as long as only iomap is supported there is no need to export it at all.
On Thu 19-05-22 01:29:21, Christoph Hellwig wrote: > > +static int balance_dirty_pages_ratelimited_flags(struct address_space *mapping, > > + bool no_wait) > > { > > This doesn't actully take flags, but a single boolean argument. So > either it needs a new name, or we actually pass a descriptiv flag. > > > +/** > > + * balance_dirty_pages_ratelimited_async - balance dirty memory state > > + * @mapping: address_space which was dirtied > > + * > > + * Processes which are dirtying memory should call in here once for each page > > + * which was newly dirtied. The function will periodically check the system's > > + * dirty state and will initiate writeback if needed. > > + * > > + * Once we're over the dirty memory limit we decrease the ratelimiting > > + * by a lot, to prevent individual processes from overshooting the limit > > + * by (ratelimit_pages) each. > > + * > > + * This is the async version of the API. It only checks if it is required to > > + * balance dirty pages. In case it needs to balance dirty pages, it returns > > + * -EAGAIN. > > + */ > > +int balance_dirty_pages_ratelimited_async(struct address_space *mapping) > > +{ > > + return balance_dirty_pages_ratelimited_flags(mapping, true); > > +} > > +EXPORT_SYMBOL(balance_dirty_pages_ratelimited_async); > > I'd much rather export the underlying > balance_dirty_pages_ratelimited_flags helper than adding a pointless > wrapper here. And as long as only iomap is supported there is no need > to export it at all. This was actually my suggestion so I take the blame ;) I have suggested this because I don't like non-static functions with bool arguments (it is unnecessarily complicated to understand what the argument means or grep for it etc.). If you don't like the wrapper, creating int balance_dirty_pages_ratelimited_flags(struct address_space *mapping, unsigned int flags) and have something like: #define BDP_NOWAIT 0x0001 is fine with me as well. Honza
On 5/19/22 1:29 AM, Christoph Hellwig wrote: >> +static int balance_dirty_pages_ratelimited_flags(struct address_space *mapping, >> + bool no_wait) >> { > > This doesn't actully take flags, but a single boolean argument. So > either it needs a new name, or we actually pass a descriptiv flag. > >> +/** >> + * balance_dirty_pages_ratelimited_async - balance dirty memory state >> + * @mapping: address_space which was dirtied >> + * >> + * Processes which are dirtying memory should call in here once for each page >> + * which was newly dirtied. The function will periodically check the system's >> + * dirty state and will initiate writeback if needed. >> + * >> + * Once we're over the dirty memory limit we decrease the ratelimiting >> + * by a lot, to prevent individual processes from overshooting the limit >> + * by (ratelimit_pages) each. >> + * >> + * This is the async version of the API. It only checks if it is required to >> + * balance dirty pages. In case it needs to balance dirty pages, it returns >> + * -EAGAIN. >> + */ >> +int balance_dirty_pages_ratelimited_async(struct address_space *mapping) >> +{ >> + return balance_dirty_pages_ratelimited_flags(mapping, true); >> +} >> +EXPORT_SYMBOL(balance_dirty_pages_ratelimited_async); > > I'd much rather export the underlying > balance_dirty_pages_ratelimited_flags helper than adding a pointless > wrapper here. And as long as only iomap is supported there is no need > to export it at all. > Thats what I originally had. I'm reverting back to it.
On 5/19/22 1:54 AM, Jan Kara wrote: > On Thu 19-05-22 01:29:21, Christoph Hellwig wrote: >>> +static int balance_dirty_pages_ratelimited_flags(struct address_space *mapping, >>> + bool no_wait) >>> { >> >> This doesn't actully take flags, but a single boolean argument. So >> either it needs a new name, or we actually pass a descriptiv flag. >> >>> +/** >>> + * balance_dirty_pages_ratelimited_async - balance dirty memory state >>> + * @mapping: address_space which was dirtied >>> + * >>> + * Processes which are dirtying memory should call in here once for each page >>> + * which was newly dirtied. The function will periodically check the system's >>> + * dirty state and will initiate writeback if needed. >>> + * >>> + * Once we're over the dirty memory limit we decrease the ratelimiting >>> + * by a lot, to prevent individual processes from overshooting the limit >>> + * by (ratelimit_pages) each. >>> + * >>> + * This is the async version of the API. It only checks if it is required to >>> + * balance dirty pages. In case it needs to balance dirty pages, it returns >>> + * -EAGAIN. >>> + */ >>> +int balance_dirty_pages_ratelimited_async(struct address_space *mapping) >>> +{ >>> + return balance_dirty_pages_ratelimited_flags(mapping, true); >>> +} >>> +EXPORT_SYMBOL(balance_dirty_pages_ratelimited_async); >> >> I'd much rather export the underlying >> balance_dirty_pages_ratelimited_flags helper than adding a pointless >> wrapper here. And as long as only iomap is supported there is no need >> to export it at all. > > This was actually my suggestion so I take the blame ;) I have suggested > this because I don't like non-static functions with bool arguments (it is > unnecessarily complicated to understand what the argument means or grep for > it etc.). If you don't like the wrapper, creating > > int balance_dirty_pages_ratelimited_flags(struct address_space *mapping, > unsigned int flags) > > and have something like: > > #define BDP_NOWAIT 0x0001 > I defined a BDP_ASYNC flag. > is fine with me as well. > > Honza
diff --git a/include/linux/writeback.h b/include/linux/writeback.h index fec248ab1fec..15eb0242d3ef 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -373,6 +373,7 @@ unsigned long wb_calc_thresh(struct bdi_writeback *wb, unsigned long thresh); void wb_update_bandwidth(struct bdi_writeback *wb); void balance_dirty_pages_ratelimited(struct address_space *mapping); +int balance_dirty_pages_ratelimited_async(struct address_space *mapping); bool wb_over_bg_thresh(struct bdi_writeback *wb); typedef int (*writepage_t)(struct page *page, struct writeback_control *wbc, diff --git a/mm/page-writeback.c b/mm/page-writeback.c index fc3b79acd90b..d6a67fc07c55 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -1851,28 +1851,18 @@ static DEFINE_PER_CPU(int, bdp_ratelimits); */ DEFINE_PER_CPU(int, dirty_throttle_leaks) = 0; -/** - * balance_dirty_pages_ratelimited - balance dirty memory state - * @mapping: address_space which was dirtied - * - * Processes which are dirtying memory should call in here once for each page - * which was newly dirtied. The function will periodically check the system's - * dirty state and will initiate writeback if needed. - * - * Once we're over the dirty memory limit we decrease the ratelimiting - * by a lot, to prevent individual processes from overshooting the limit - * by (ratelimit_pages) each. - */ -void balance_dirty_pages_ratelimited(struct address_space *mapping) +static int balance_dirty_pages_ratelimited_flags(struct address_space *mapping, + bool no_wait) { struct inode *inode = mapping->host; struct backing_dev_info *bdi = inode_to_bdi(inode); struct bdi_writeback *wb = NULL; int ratelimit; + int ret = 0; int *p; if (!(bdi->capabilities & BDI_CAP_WRITEBACK)) - return; + return ret; if (inode_cgwb_enabled(inode)) wb = wb_get_create_current(bdi, GFP_KERNEL); @@ -1912,12 +1902,52 @@ void balance_dirty_pages_ratelimited(struct address_space *mapping) preempt_enable(); if (unlikely(current->nr_dirtied >= ratelimit)) - balance_dirty_pages(wb, current->nr_dirtied, false); + balance_dirty_pages(wb, current->nr_dirtied, no_wait); wb_put(wb); + return ret; +} + +/** + * balance_dirty_pages_ratelimited - balance dirty memory state + * @mapping: address_space which was dirtied + * + * Processes which are dirtying memory should call in here once for each page + * which was newly dirtied. The function will periodically check the system's + * dirty state and will initiate writeback if needed. + * + * Once we're over the dirty memory limit we decrease the ratelimiting + * by a lot, to prevent individual processes from overshooting the limit + * by (ratelimit_pages) each. + */ +void balance_dirty_pages_ratelimited(struct address_space *mapping) +{ + balance_dirty_pages_ratelimited_flags(mapping, false); } EXPORT_SYMBOL(balance_dirty_pages_ratelimited); +/** + * balance_dirty_pages_ratelimited_async - balance dirty memory state + * @mapping: address_space which was dirtied + * + * Processes which are dirtying memory should call in here once for each page + * which was newly dirtied. The function will periodically check the system's + * dirty state and will initiate writeback if needed. + * + * Once we're over the dirty memory limit we decrease the ratelimiting + * by a lot, to prevent individual processes from overshooting the limit + * by (ratelimit_pages) each. + * + * This is the async version of the API. It only checks if it is required to + * balance dirty pages. In case it needs to balance dirty pages, it returns + * -EAGAIN. + */ +int balance_dirty_pages_ratelimited_async(struct address_space *mapping) +{ + return balance_dirty_pages_ratelimited_flags(mapping, true); +} +EXPORT_SYMBOL(balance_dirty_pages_ratelimited_async); + /** * wb_over_bg_thresh - does @wb need to be written back? * @wb: bdi_writeback of interest
This adds the helper function balance_dirty_pages_ratelimited_flags(). It adds the parameter no_wait to balance_dirty_pages_ratelimited(). For async buffered writes no_wait will be true. A new function called balance_dirty_pages_ratelimited_async() is introduced that calls balance_dirty_pages_ratelimited_flags with no_wait set to true. If write throttling is enabled, it retuns -EAGAIN, so the write request can be punted to the io-uring worker. For non-async writes the current behavior is maintained. Signed-off-by: Stefan Roesch <shr@fb.com> --- include/linux/writeback.h | 1 + mm/page-writeback.c | 60 +++++++++++++++++++++++++++++---------- 2 files changed, 46 insertions(+), 15 deletions(-)