| Message ID | 20230403220337.443510-2-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cgroup: eliminate atomic rstat | expand |
On Mon, Apr 03, 2023 at 10:03:33PM +0000, Yosry Ahmed <yosryahmed@google.com> wrote: > wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat > flush, which can be expensive on large systems. Currently, > wb_writeback() calls wb_over_bg_thresh() within a lock section, so we > have to make the rstat flush atomically. On systems with a lot of > cpus/cgroups, this can cause us to disable irqs for a long time, > potentially causing problems. > > Move the call to wb_over_bg_thresh() outside the lock section in > preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic. > The list_empty(&wb->work_list) should be okay outside the lock section > of wb->list_lock as it is protected by a separate lock (wb->work_lock), > and wb_over_bg_thresh() doesn't seem like it is modifying any of the b_* > lists the wb->list_lock is protecting. Also, the loop seems to be > already releasing and reacquring the lock, so this refactoring looks > safe. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > --- > fs/fs-writeback.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index 195dc23e0d831..012357bc8daa3 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -2021,7 +2021,6 @@ static long wb_writeback(struct bdi_writeback *wb, > struct blk_plug plug; > > blk_start_plug(&plug); > - spin_lock(&wb->list_lock); > for (;;) { > /* > * Stop writeback when nr_pages has been consumed > @@ -2046,6 +2045,9 @@ static long wb_writeback(struct bdi_writeback *wb, > if (work->for_background && !wb_over_bg_thresh(wb)) > break; > > + > + spin_lock(&wb->list_lock); > + > /* > * Kupdate and background works are special and we want to > * include all inodes that need writing. Livelock avoidance is > @@ -2075,13 +2077,19 @@ static long wb_writeback(struct bdi_writeback *wb, > * mean the overall work is done. So we keep looping as long > * as made some progress on cleaning pages or inodes. > */ > - if (progress) > + if (progress) { > + spin_unlock(&wb->list_lock); > continue; > + } > + This would release wb->list_lock temporarily with progress but that's already not held continuously due to writeback_sb_inodes(). Holding the lock could even be shortened by taking it later after trace_writeback_start(). Altogether, the change looks OK, Reviewed-by: Michal Koutný <mkoutny@suse.com>
+Jens & Jan The patch looks good but it would be nice to pass this patch through the eyes of experts of this area. On Mon, Apr 3, 2023 at 3:03 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat > flush, which can be expensive on large systems. Currently, > wb_writeback() calls wb_over_bg_thresh() within a lock section, so we > have to make the rstat flush atomically. On systems with a lot of > cpus/cgroups, this can cause us to disable irqs for a long time, > potentially causing problems. > > Move the call to wb_over_bg_thresh() outside the lock section in > preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic. > The list_empty(&wb->work_list) should be okay outside the lock section > of wb->list_lock as it is protected by a separate lock (wb->work_lock), > and wb_over_bg_thresh() doesn't seem like it is modifying any of the b_* > lists the wb->list_lock is protecting. Also, the loop seems to be > already releasing and reacquring the lock, so this refactoring looks > safe. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > --- > fs/fs-writeback.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index 195dc23e0d831..012357bc8daa3 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -2021,7 +2021,6 @@ static long wb_writeback(struct bdi_writeback *wb, > struct blk_plug plug; > > blk_start_plug(&plug); > - spin_lock(&wb->list_lock); > for (;;) { > /* > * Stop writeback when nr_pages has been consumed > @@ -2046,6 +2045,9 @@ static long wb_writeback(struct bdi_writeback *wb, > if (work->for_background && !wb_over_bg_thresh(wb)) > break; > > + > + spin_lock(&wb->list_lock); > + > /* > * Kupdate and background works are special and we want to > * include all inodes that need writing. Livelock avoidance is > @@ -2075,13 +2077,19 @@ static long wb_writeback(struct bdi_writeback *wb, > * mean the overall work is done. So we keep looping as long > * as made some progress on cleaning pages or inodes. > */ > - if (progress) > + if (progress) { > + spin_unlock(&wb->list_lock); > continue; > + } > + > /* > * No more inodes for IO, bail > */ > - if (list_empty(&wb->b_more_io)) > + if (list_empty(&wb->b_more_io)) { > + spin_unlock(&wb->list_lock); > break; > + } > + > /* > * Nothing written. Wait for some inode to > * become available for writeback. Otherwise > @@ -2093,9 +2101,7 @@ static long wb_writeback(struct bdi_writeback *wb, > spin_unlock(&wb->list_lock); > /* This function drops i_lock... */ > inode_sleep_on_writeback(inode); > - spin_lock(&wb->list_lock); > } > - spin_unlock(&wb->list_lock); > blk_finish_plug(&plug); > > return nr_pages - work->nr_pages; > -- > 2.40.0.348.gf938b09366-goog >
On Thu, Apr 20, 2023 at 11:53 AM Shakeel Butt <shakeelb@google.com> wrote: > > +Jens & Jan > > The patch looks good but it would be nice to pass this patch through > the eyes of experts of this area. Thanks for taking a look and CC'ing folks. I will make sure to include them in the next rounds as well. FWIW, Jens & Jan did not show up when I ran scripts/get_maintainers.ph if I remember correctly. > > On Mon, Apr 3, 2023 at 3:03 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > > wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat > > flush, which can be expensive on large systems. Currently, > > wb_writeback() calls wb_over_bg_thresh() within a lock section, so we > > have to make the rstat flush atomically. On systems with a lot of > > cpus/cgroups, this can cause us to disable irqs for a long time, > > potentially causing problems. > > > > Move the call to wb_over_bg_thresh() outside the lock section in > > preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic. > > The list_empty(&wb->work_list) should be okay outside the lock section > > of wb->list_lock as it is protected by a separate lock (wb->work_lock), > > and wb_over_bg_thresh() doesn't seem like it is modifying any of the b_* > > lists the wb->list_lock is protecting. Also, the loop seems to be > > already releasing and reacquring the lock, so this refactoring looks > > safe. > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > --- > > fs/fs-writeback.c | 16 +++++++++++----- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > > index 195dc23e0d831..012357bc8daa3 100644 > > --- a/fs/fs-writeback.c > > +++ b/fs/fs-writeback.c > > @@ -2021,7 +2021,6 @@ static long wb_writeback(struct bdi_writeback *wb, > > struct blk_plug plug; > > > > blk_start_plug(&plug); > > - spin_lock(&wb->list_lock); > > for (;;) { > > /* > > * Stop writeback when nr_pages has been consumed > > @@ -2046,6 +2045,9 @@ static long wb_writeback(struct bdi_writeback *wb, > > if (work->for_background && !wb_over_bg_thresh(wb)) > > break; > > > > + > > + spin_lock(&wb->list_lock); > > + > > /* > > * Kupdate and background works are special and we want to > > * include all inodes that need writing. Livelock avoidance is > > @@ -2075,13 +2077,19 @@ static long wb_writeback(struct bdi_writeback *wb, > > * mean the overall work is done. So we keep looping as long > > * as made some progress on cleaning pages or inodes. > > */ > > - if (progress) > > + if (progress) { > > + spin_unlock(&wb->list_lock); > > continue; > > + } > > + > > /* > > * No more inodes for IO, bail > > */ > > - if (list_empty(&wb->b_more_io)) > > + if (list_empty(&wb->b_more_io)) { > > + spin_unlock(&wb->list_lock); > > break; > > + } > > + > > /* > > * Nothing written. Wait for some inode to > > * become available for writeback. Otherwise > > @@ -2093,9 +2101,7 @@ static long wb_writeback(struct bdi_writeback *wb, > > spin_unlock(&wb->list_lock); > > /* This function drops i_lock... */ > > inode_sleep_on_writeback(inode); > > - spin_lock(&wb->list_lock); > > } > > - spin_unlock(&wb->list_lock); > > blk_finish_plug(&plug); > > > > return nr_pages - work->nr_pages; > > -- > > 2.40.0.348.gf938b09366-goog > >
On Wed, Apr 19, 2023 at 4:38 AM Michal Koutný <mkoutny@suse.com> wrote: > > On Mon, Apr 03, 2023 at 10:03:33PM +0000, Yosry Ahmed <yosryahmed@google.com> wrote: > > wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat > > flush, which can be expensive on large systems. Currently, > > wb_writeback() calls wb_over_bg_thresh() within a lock section, so we > > have to make the rstat flush atomically. On systems with a lot of > > cpus/cgroups, this can cause us to disable irqs for a long time, > > potentially causing problems. > > > > Move the call to wb_over_bg_thresh() outside the lock section in > > preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic. > > The list_empty(&wb->work_list) should be okay outside the lock section > > of wb->list_lock as it is protected by a separate lock (wb->work_lock), > > and wb_over_bg_thresh() doesn't seem like it is modifying any of the b_* > > lists the wb->list_lock is protecting. Also, the loop seems to be > > already releasing and reacquring the lock, so this refactoring looks > > safe. > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > --- > > fs/fs-writeback.c | 16 +++++++++++----- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > > index 195dc23e0d831..012357bc8daa3 100644 > > --- a/fs/fs-writeback.c > > +++ b/fs/fs-writeback.c > > @@ -2021,7 +2021,6 @@ static long wb_writeback(struct bdi_writeback *wb, > > struct blk_plug plug; > > > > blk_start_plug(&plug); > > - spin_lock(&wb->list_lock); > > for (;;) { > > /* > > * Stop writeback when nr_pages has been consumed > > @@ -2046,6 +2045,9 @@ static long wb_writeback(struct bdi_writeback *wb, > > if (work->for_background && !wb_over_bg_thresh(wb)) > > break; > > > > + > > + spin_lock(&wb->list_lock); > > + > > /* > > * Kupdate and background works are special and we want to > > * include all inodes that need writing. Livelock avoidance is > > @@ -2075,13 +2077,19 @@ static long wb_writeback(struct bdi_writeback *wb, > > * mean the overall work is done. So we keep looping as long > > * as made some progress on cleaning pages or inodes. > > */ > > - if (progress) > > + if (progress) { > > + spin_unlock(&wb->list_lock); > > continue; > > + } > > + > > This would release wb->list_lock temporarily with progress but that's > already not held continuously due to writeback_sb_inodes(). > Holding the lock could even be shortened by taking it later after > trace_writeback_start(). > > Altogether, the change looks OK, > Reviewed-by: Michal Koutný <mkoutny@suse.com> Thanks for taking a look! >
On Mon 03-04-23 22:03:33, Yosry Ahmed wrote: > wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat > flush, which can be expensive on large systems. Currently, > wb_writeback() calls wb_over_bg_thresh() within a lock section, so we > have to make the rstat flush atomically. On systems with a lot of > cpus/cgroups, this can cause us to disable irqs for a long time, > potentially causing problems. > > Move the call to wb_over_bg_thresh() outside the lock section in > preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic. > The list_empty(&wb->work_list) should be okay outside the lock section > of wb->list_lock as it is protected by a separate lock (wb->work_lock), > and wb_over_bg_thresh() doesn't seem like it is modifying any of the b_* > lists the wb->list_lock is protecting. Also, the loop seems to be > already releasing and reacquring the lock, so this refactoring looks > safe. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> The patch looks good to me. Nice find. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/fs-writeback.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index 195dc23e0d831..012357bc8daa3 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -2021,7 +2021,6 @@ static long wb_writeback(struct bdi_writeback *wb, > struct blk_plug plug; > > blk_start_plug(&plug); > - spin_lock(&wb->list_lock); > for (;;) { > /* > * Stop writeback when nr_pages has been consumed > @@ -2046,6 +2045,9 @@ static long wb_writeback(struct bdi_writeback *wb, > if (work->for_background && !wb_over_bg_thresh(wb)) > break; > > + > + spin_lock(&wb->list_lock); > + > /* > * Kupdate and background works are special and we want to > * include all inodes that need writing. Livelock avoidance is > @@ -2075,13 +2077,19 @@ static long wb_writeback(struct bdi_writeback *wb, > * mean the overall work is done. So we keep looping as long > * as made some progress on cleaning pages or inodes. > */ > - if (progress) > + if (progress) { > + spin_unlock(&wb->list_lock); > continue; > + } > + > /* > * No more inodes for IO, bail > */ > - if (list_empty(&wb->b_more_io)) > + if (list_empty(&wb->b_more_io)) { > + spin_unlock(&wb->list_lock); > break; > + } > + > /* > * Nothing written. Wait for some inode to > * become available for writeback. Otherwise > @@ -2093,9 +2101,7 @@ static long wb_writeback(struct bdi_writeback *wb, > spin_unlock(&wb->list_lock); > /* This function drops i_lock... */ > inode_sleep_on_writeback(inode); > - spin_lock(&wb->list_lock); > } > - spin_unlock(&wb->list_lock); > blk_finish_plug(&plug); > > return nr_pages - work->nr_pages; > -- > 2.40.0.348.gf938b09366-goog >
On Fri, Apr 21, 2023 at 1:53 AM Jan Kara <jack@suse.cz> wrote: > > On Mon 03-04-23 22:03:33, Yosry Ahmed wrote: > > wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat > > flush, which can be expensive on large systems. Currently, > > wb_writeback() calls wb_over_bg_thresh() within a lock section, so we > > have to make the rstat flush atomically. On systems with a lot of > > cpus/cgroups, this can cause us to disable irqs for a long time, > > potentially causing problems. > > > > Move the call to wb_over_bg_thresh() outside the lock section in > > preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic. > > The list_empty(&wb->work_list) should be okay outside the lock section > > of wb->list_lock as it is protected by a separate lock (wb->work_lock), > > and wb_over_bg_thresh() doesn't seem like it is modifying any of the b_* > > lists the wb->list_lock is protecting. Also, the loop seems to be > > already releasing and reacquring the lock, so this refactoring looks > > safe. > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > The patch looks good to me. Nice find. Feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> Thanks for taking a look! > > Honza > > > --- > > fs/fs-writeback.c | 16 +++++++++++----- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > > index 195dc23e0d831..012357bc8daa3 100644 > > --- a/fs/fs-writeback.c > > +++ b/fs/fs-writeback.c > > @@ -2021,7 +2021,6 @@ static long wb_writeback(struct bdi_writeback *wb, > > struct blk_plug plug; > > > > blk_start_plug(&plug); > > - spin_lock(&wb->list_lock); > > for (;;) { > > /* > > * Stop writeback when nr_pages has been consumed > > @@ -2046,6 +2045,9 @@ static long wb_writeback(struct bdi_writeback *wb, > > if (work->for_background && !wb_over_bg_thresh(wb)) > > break; > > > > + > > + spin_lock(&wb->list_lock); > > + > > /* > > * Kupdate and background works are special and we want to > > * include all inodes that need writing. Livelock avoidance is > > @@ -2075,13 +2077,19 @@ static long wb_writeback(struct bdi_writeback *wb, > > * mean the overall work is done. So we keep looping as long > > * as made some progress on cleaning pages or inodes. > > */ > > - if (progress) > > + if (progress) { > > + spin_unlock(&wb->list_lock); > > continue; > > + } > > + > > /* > > * No more inodes for IO, bail > > */ > > - if (list_empty(&wb->b_more_io)) > > + if (list_empty(&wb->b_more_io)) { > > + spin_unlock(&wb->list_lock); > > break; > > + } > > + > > /* > > * Nothing written. Wait for some inode to > > * become available for writeback. Otherwise > > @@ -2093,9 +2101,7 @@ static long wb_writeback(struct bdi_writeback *wb, > > spin_unlock(&wb->list_lock); > > /* This function drops i_lock... */ > > inode_sleep_on_writeback(inode); > > - spin_lock(&wb->list_lock); > > } > > - spin_unlock(&wb->list_lock); > > blk_finish_plug(&plug); > > > > return nr_pages - work->nr_pages; > > -- > > 2.40.0.348.gf938b09366-goog > > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index 195dc23e0d831..012357bc8daa3 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -2021,7 +2021,6 @@ static long wb_writeback(struct bdi_writeback *wb, struct blk_plug plug; blk_start_plug(&plug); - spin_lock(&wb->list_lock); for (;;) { /* * Stop writeback when nr_pages has been consumed @@ -2046,6 +2045,9 @@ static long wb_writeback(struct bdi_writeback *wb, if (work->for_background && !wb_over_bg_thresh(wb)) break; + + spin_lock(&wb->list_lock); + /* * Kupdate and background works are special and we want to * include all inodes that need writing. Livelock avoidance is @@ -2075,13 +2077,19 @@ static long wb_writeback(struct bdi_writeback *wb, * mean the overall work is done. So we keep looping as long * as made some progress on cleaning pages or inodes. */ - if (progress) + if (progress) { + spin_unlock(&wb->list_lock); continue; + } + /* * No more inodes for IO, bail */ - if (list_empty(&wb->b_more_io)) + if (list_empty(&wb->b_more_io)) { + spin_unlock(&wb->list_lock); break; + } + /* * Nothing written. Wait for some inode to * become available for writeback. Otherwise @@ -2093,9 +2101,7 @@ static long wb_writeback(struct bdi_writeback *wb, spin_unlock(&wb->list_lock); /* This function drops i_lock... */ inode_sleep_on_writeback(inode); - spin_lock(&wb->list_lock); } - spin_unlock(&wb->list_lock); blk_finish_plug(&plug); return nr_pages - work->nr_pages;
wb_over_bg_thresh() calls mem_cgroup_wb_stats() which invokes an rstat flush, which can be expensive on large systems. Currently, wb_writeback() calls wb_over_bg_thresh() within a lock section, so we have to make the rstat flush atomically. On systems with a lot of cpus/cgroups, this can cause us to disable irqs for a long time, potentially causing problems. Move the call to wb_over_bg_thresh() outside the lock section in preparation to make the rstat flush in mem_cgroup_wb_stats() non-atomic. The list_empty(&wb->work_list) should be okay outside the lock section of wb->list_lock as it is protected by a separate lock (wb->work_lock), and wb_over_bg_thresh() doesn't seem like it is modifying any of the b_* lists the wb->list_lock is protecting. Also, the loop seems to be already releasing and reacquring the lock, so this refactoring looks safe. Signed-off-by: Yosry Ahmed <yosryahmed@google.com> --- fs/fs-writeback.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)