| Message ID | 1432329245-5844-37-git-send-email-tj@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri 22-05-15 17:13:50, Tejun Heo wrote: > This will be used to implement bdi-wide operations which should be > distributed across all its cgroup bdi_writebacks. > > Signed-off-by: Tejun Heo <tj@kernel.org> > Cc: Jens Axboe <axboe@kernel.dk> > Cc: Jan Kara <jack@suse.cz> One comment below. > @@ -445,6 +500,14 @@ static inline void wb_blkcg_offline(struct blkcg *blkcg) > { > } > > +struct wb_iter { > + int next_id; > +}; > + > +#define bdi_for_each_wb(wb_cur, bdi, iter, start_blkcg_id) \ > + for ((iter)->next_id = (start_blkcg_id); \ > + ({ (wb_cur) = !(iter)->next_id++ ? &(bdi)->wb : NULL; }); ) > + This looks quite confusing. Won't it be easier to understand as: struct wb_iter { } __attribute__ ((unused)); #define bdi_for_each_wb(wb_cur, bdi, iter, start_blkcg_id) \ if (((wb_cur) = (!start_blkcg_id ? &(bdi)->wb : NULL))) Honza
On Wed, Jul 01, 2015 at 09:27:57AM +0200, Jan Kara wrote: > > +#define bdi_for_each_wb(wb_cur, bdi, iter, start_blkcg_id) \ > > + for ((iter)->next_id = (start_blkcg_id); \ > > + ({ (wb_cur) = !(iter)->next_id++ ? &(bdi)->wb : NULL; }); ) > > + > > This looks quite confusing. Won't it be easier to understand as: > > struct wb_iter { > } __attribute__ ((unused)); > > #define bdi_for_each_wb(wb_cur, bdi, iter, start_blkcg_id) \ > if (((wb_cur) = (!start_blkcg_id ? &(bdi)->wb : NULL))) But then break or continue wouldn't work as expected. It can get really confusing when it's wrapped by an outer loop. Thanks.
On Wed 01-07-15 22:22:26, Tejun Heo wrote: > On Wed, Jul 01, 2015 at 09:27:57AM +0200, Jan Kara wrote: > > > +#define bdi_for_each_wb(wb_cur, bdi, iter, start_blkcg_id) \ > > > + for ((iter)->next_id = (start_blkcg_id); \ > > > + ({ (wb_cur) = !(iter)->next_id++ ? &(bdi)->wb : NULL; }); ) > > > + > > > > This looks quite confusing. Won't it be easier to understand as: > > > > struct wb_iter { > > } __attribute__ ((unused)); > > > > #define bdi_for_each_wb(wb_cur, bdi, iter, start_blkcg_id) \ > > if (((wb_cur) = (!start_blkcg_id ? &(bdi)->wb : NULL))) > > But then break or continue wouldn't work as expected. It can get > really confusing when it's wrapped by an outer loop. That's a good point. Thanks for explanation. Maybe add a comment like: /* * We use use this seemingly complicated 'for' loop so that 'break' and * 'continue' continue to work as expected. */ Honza
On Fri, Jul 03, 2015 at 02:26:27PM +0200, Jan Kara wrote: > That's a good point. Thanks for explanation. Maybe add a comment like: > /* > * We use use this seemingly complicated 'for' loop so that 'break' and > * 'continue' continue to work as expected. > */ This kinda feel superflous for me. This is something true for all iteration wrappers which falls within the area of well-established convention, I think. If it's doing something weird like combining if-else clause to do post-conditional processing, sure, but this is really kinda standard. Thanks.
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h index 0839e44..c797980 100644 --- a/include/linux/backing-dev.h +++ b/include/linux/backing-dev.h @@ -383,6 +383,61 @@ static inline struct bdi_writeback *inode_to_wb(struct inode *inode) return inode->i_wb; } +struct wb_iter { + int start_blkcg_id; + struct radix_tree_iter tree_iter; + void **slot; +}; + +static inline struct bdi_writeback *__wb_iter_next(struct wb_iter *iter, + struct backing_dev_info *bdi) +{ + struct radix_tree_iter *titer = &iter->tree_iter; + + WARN_ON_ONCE(!rcu_read_lock_held()); + + if (iter->start_blkcg_id >= 0) { + iter->slot = radix_tree_iter_init(titer, iter->start_blkcg_id); + iter->start_blkcg_id = -1; + } else { + iter->slot = radix_tree_next_slot(iter->slot, titer, 0); + } + + if (!iter->slot) + iter->slot = radix_tree_next_chunk(&bdi->cgwb_tree, titer, 0); + if (iter->slot) + return *iter->slot; + return NULL; +} + +static inline struct bdi_writeback *__wb_iter_init(struct wb_iter *iter, + struct backing_dev_info *bdi, + int start_blkcg_id) +{ + iter->start_blkcg_id = start_blkcg_id; + + if (start_blkcg_id) + return __wb_iter_next(iter, bdi); + else + return &bdi->wb; +} + +/** + * bdi_for_each_wb - walk all wb's of a bdi in ascending blkcg ID order + * @wb_cur: cursor struct bdi_writeback pointer + * @bdi: bdi to walk wb's of + * @iter: pointer to struct wb_iter to be used as iteration buffer + * @start_blkcg_id: blkcg ID to start iteration from + * + * Iterate @wb_cur through the wb's (bdi_writeback's) of @bdi in ascending + * blkcg ID order starting from @start_blkcg_id. @iter is struct wb_iter + * to be used as temp storage during iteration. rcu_read_lock() must be + * held throughout iteration. + */ +#define bdi_for_each_wb(wb_cur, bdi, iter, start_blkcg_id) \ + for ((wb_cur) = __wb_iter_init(iter, bdi, start_blkcg_id); \ + (wb_cur); (wb_cur) = __wb_iter_next(iter, bdi)) + #else /* CONFIG_CGROUP_WRITEBACK */ static inline bool inode_cgwb_enabled(struct inode *inode) @@ -445,6 +500,14 @@ static inline void wb_blkcg_offline(struct blkcg *blkcg) { } +struct wb_iter { + int next_id; +}; + +#define bdi_for_each_wb(wb_cur, bdi, iter, start_blkcg_id) \ + for ((iter)->next_id = (start_blkcg_id); \ + ({ (wb_cur) = !(iter)->next_id++ ? &(bdi)->wb : NULL; }); ) + static inline int inode_congested(struct inode *inode, int cong_bits) { return wb_congested(&inode_to_bdi(inode)->wb, cong_bits);
This will be used to implement bdi-wide operations which should be distributed across all its cgroup bdi_writebacks. Signed-off-by: Tejun Heo <tj@kernel.org> Cc: Jens Axboe <axboe@kernel.dk> Cc: Jan Kara <jack@suse.cz> --- include/linux/backing-dev.h | 63 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)