| Message ID | 163157838437.13293.15392955714346973750.stgit@noble.brown (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | congestion_wait() and GFP_NOFAIL | expand |
On Tue 14-09-21 10:13:04, Neil Brown wrote: > Only 4 subsystems call set_bdi_congested() or clear_bdi_congested(): > block/pktcdvd, fs/ceph fs/fuse fs/nfs > > It may make sense to use congestion_wait() or wait_iff_congested() > within these subsystems, but they have no value outside of these. > > Add documentation comments to these functions to discourage further use. This is an unfortunate state. The MM layer still relies on the API. While adding a documentation to clarify the current status can stop more usage I am wondering what is a real alternative. My experience tells me that a lack of real alternative will lead to new creative ways of doing things instead. > Signed-off-by: NeilBrown <neilb@suse.de> > --- > include/linux/backing-dev.h | 7 +++++++ > mm/backing-dev.c | 9 +++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h > index ac7f231b8825..cc9513840351 100644 > --- a/include/linux/backing-dev.h > +++ b/include/linux/backing-dev.h > @@ -153,6 +153,13 @@ static inline int wb_congested(struct bdi_writeback *wb, int cong_bits) > return wb->congested & cong_bits; > } > > +/* NOTE congestion_wait() and wait_iff_congested() are > + * largely useless except as documentation. > + * congestion_wait() will (almost) always wait for the given timeout. > + * wait_iff_congested() will (almost) never wait, but will call > + * cond_resched(). > + * Were possible an alternative waiting strategy should be found. > + */ > long congestion_wait(int sync, long timeout); > long wait_iff_congested(int sync, long timeout); > > diff --git a/mm/backing-dev.c b/mm/backing-dev.c > index 4a9d4e27d0d9..53472ab38796 100644 > --- a/mm/backing-dev.c > +++ b/mm/backing-dev.c > @@ -1023,6 +1023,11 @@ EXPORT_SYMBOL(set_bdi_congested); > * Waits for up to @timeout jiffies for a backing_dev (any backing_dev) to exit > * write congestion. If no backing_devs are congested then just wait for the > * next write to be completed. > + * > + * NOTE: in the current implementation, hardly any backing_devs are ever > + * marked as congested, and write-completion is rarely reported (see calls > + * to clear_bdi_congested). So this should not be assumed to ever wake before > + * the timeout. > */ > long congestion_wait(int sync, long timeout) > { > @@ -1054,6 +1059,10 @@ EXPORT_SYMBOL(congestion_wait); > * The return value is 0 if the sleep is for the full timeout. Otherwise, > * it is the number of jiffies that were still remaining when the function > * returned. return_value == timeout implies the function did not sleep. > + * > + * NOTE: in the current implementation, hardly any backing_devs are ever > + * marked as congested, and write-completion is rarely reported (see calls > + * to clear_bdi_congested). So this should not be assumed to sleep at all. > */ > long wait_iff_congested(int sync, long timeout) > { >
On Wed, 15 Sep 2021, Michal Hocko wrote: > On Tue 14-09-21 10:13:04, Neil Brown wrote: > > Only 4 subsystems call set_bdi_congested() or clear_bdi_congested(): > > block/pktcdvd, fs/ceph fs/fuse fs/nfs > > > > It may make sense to use congestion_wait() or wait_iff_congested() > > within these subsystems, but they have no value outside of these. > > > > Add documentation comments to these functions to discourage further use. > > This is an unfortunate state. The MM layer still relies on the API. > While adding a documentation to clarify the current status can stop more > usage I am wondering what is a real alternative. My experience tells me > that a lack of real alternative will lead to new creative ways of doing > things instead. That is a valid concern. Discouraging the use of an interface without providing a clear alternative risks people doing worse things. At lease if people continue to use congestion_wait(), then we will be able to find those uses when we are able to provide a better approach. I'll drop this patch. Thanks, NeilBrown > > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > include/linux/backing-dev.h | 7 +++++++ > > mm/backing-dev.c | 9 +++++++++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h > > index ac7f231b8825..cc9513840351 100644 > > --- a/include/linux/backing-dev.h > > +++ b/include/linux/backing-dev.h > > @@ -153,6 +153,13 @@ static inline int wb_congested(struct bdi_writeback *wb, int cong_bits) > > return wb->congested & cong_bits; > > } > > > > +/* NOTE congestion_wait() and wait_iff_congested() are > > + * largely useless except as documentation. > > + * congestion_wait() will (almost) always wait for the given timeout. > > + * wait_iff_congested() will (almost) never wait, but will call > > + * cond_resched(). > > + * Were possible an alternative waiting strategy should be found. > > + */ > > long congestion_wait(int sync, long timeout); > > long wait_iff_congested(int sync, long timeout); > > > > diff --git a/mm/backing-dev.c b/mm/backing-dev.c > > index 4a9d4e27d0d9..53472ab38796 100644 > > --- a/mm/backing-dev.c > > +++ b/mm/backing-dev.c > > @@ -1023,6 +1023,11 @@ EXPORT_SYMBOL(set_bdi_congested); > > * Waits for up to @timeout jiffies for a backing_dev (any backing_dev) to exit > > * write congestion. If no backing_devs are congested then just wait for the > > * next write to be completed. > > + * > > + * NOTE: in the current implementation, hardly any backing_devs are ever > > + * marked as congested, and write-completion is rarely reported (see calls > > + * to clear_bdi_congested). So this should not be assumed to ever wake before > > + * the timeout. > > */ > > long congestion_wait(int sync, long timeout) > > { > > @@ -1054,6 +1059,10 @@ EXPORT_SYMBOL(congestion_wait); > > * The return value is 0 if the sleep is for the full timeout. Otherwise, > > * it is the number of jiffies that were still remaining when the function > > * returned. return_value == timeout implies the function did not sleep. > > + * > > + * NOTE: in the current implementation, hardly any backing_devs are ever > > + * marked as congested, and write-completion is rarely reported (see calls > > + * to clear_bdi_congested). So this should not be assumed to sleep at all. > > */ > > long wait_iff_congested(int sync, long timeout) > > { > > > > -- > Michal Hocko > SUSE Labs > >
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h index ac7f231b8825..cc9513840351 100644 --- a/include/linux/backing-dev.h +++ b/include/linux/backing-dev.h @@ -153,6 +153,13 @@ static inline int wb_congested(struct bdi_writeback *wb, int cong_bits) return wb->congested & cong_bits; } +/* NOTE congestion_wait() and wait_iff_congested() are + * largely useless except as documentation. + * congestion_wait() will (almost) always wait for the given timeout. + * wait_iff_congested() will (almost) never wait, but will call + * cond_resched(). + * Were possible an alternative waiting strategy should be found. + */ long congestion_wait(int sync, long timeout); long wait_iff_congested(int sync, long timeout); diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 4a9d4e27d0d9..53472ab38796 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -1023,6 +1023,11 @@ EXPORT_SYMBOL(set_bdi_congested); * Waits for up to @timeout jiffies for a backing_dev (any backing_dev) to exit * write congestion. If no backing_devs are congested then just wait for the * next write to be completed. + * + * NOTE: in the current implementation, hardly any backing_devs are ever + * marked as congested, and write-completion is rarely reported (see calls + * to clear_bdi_congested). So this should not be assumed to ever wake before + * the timeout. */ long congestion_wait(int sync, long timeout) { @@ -1054,6 +1059,10 @@ EXPORT_SYMBOL(congestion_wait); * The return value is 0 if the sleep is for the full timeout. Otherwise, * it is the number of jiffies that were still remaining when the function * returned. return_value == timeout implies the function did not sleep. + * + * NOTE: in the current implementation, hardly any backing_devs are ever + * marked as congested, and write-completion is rarely reported (see calls + * to clear_bdi_congested). So this should not be assumed to sleep at all. */ long wait_iff_congested(int sync, long timeout) {
Only 4 subsystems call set_bdi_congested() or clear_bdi_congested(): block/pktcdvd, fs/ceph fs/fuse fs/nfs It may make sense to use congestion_wait() or wait_iff_congested() within these subsystems, but they have no value outside of these. Add documentation comments to these functions to discourage further use. Signed-off-by: NeilBrown <neilb@suse.de> --- include/linux/backing-dev.h | 7 +++++++ mm/backing-dev.c | 9 +++++++++ 2 files changed, 16 insertions(+)