| Message ID | 20220526163604.32736-13-logang@deltatee.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bug fixes for mdadm tests | expand |
On Thu, May 26, 2022 at 10:35:59AM -0600, Logan Gunthorpe wrote: > Move struct r5l_log definition to raid5-log.h. While this reduces > encapsulation, it is necessary for the definition of r5l_log to be > public so that rcu_access_pointer() can be used on conf-log in the > next patch. > > rcu_access_pointer(p) doesn't technically dereference the log pointer > however, it does use typeof(*p) and some older GCC versions (anything > earlier than gcc-10) will wrongly try to dereference the structure: > > include/linux/rcupdate.h:384:9: error: dereferencing pointer to > incomplete type ‘struct r5l_log’ > > typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > ^ > > include/linux/rcupdate.h:495:31: note: in expansion of > macro ‘__rcu_access_pointer’ > > #define rcu_access_pointer(p) __rcu_access_pointer((p), > __UNIQUE_ID(rcu), __rcu) > > To prevent this, simply provide the definition where > rcu_access_pointer() may be used. What about just moving any code that does the rcu_access_pointer on conf->log to raid5-cache.c and doing an out of line call for it instead?
On 2022-05-29 23:59, Christoph Hellwig wrote: > On Thu, May 26, 2022 at 10:35:59AM -0600, Logan Gunthorpe wrote: >> Move struct r5l_log definition to raid5-log.h. While this reduces >> encapsulation, it is necessary for the definition of r5l_log to be >> public so that rcu_access_pointer() can be used on conf-log in the >> next patch. >> >> rcu_access_pointer(p) doesn't technically dereference the log pointer >> however, it does use typeof(*p) and some older GCC versions (anything >> earlier than gcc-10) will wrongly try to dereference the structure: >> >> include/linux/rcupdate.h:384:9: error: dereferencing pointer to >> incomplete type ‘struct r5l_log’ >> >> typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ >> ^ >> >> include/linux/rcupdate.h:495:31: note: in expansion of >> macro ‘__rcu_access_pointer’ >> >> #define rcu_access_pointer(p) __rcu_access_pointer((p), >> __UNIQUE_ID(rcu), __rcu) >> >> To prevent this, simply provide the definition where >> rcu_access_pointer() may be used. > > What about just moving any code that does the rcu_access_pointer on > conf->log to raid5-cache.c and doing an out of line call for it > instead? I guess we could do that. All the inline functions in raid5-log.h are there to choose between the r5l or the ppl implementaiton. So it that would mean the r5l implementation would probably be inlined and ppl would be doing a second out of line call. Not sure if that matters, but it seems a little odd. Logan
On Mon, May 30, 2022 at 8:48 AM Logan Gunthorpe <logang@deltatee.com> wrote: > > > > On 2022-05-29 23:59, Christoph Hellwig wrote: > > On Thu, May 26, 2022 at 10:35:59AM -0600, Logan Gunthorpe wrote: > >> Move struct r5l_log definition to raid5-log.h. While this reduces > >> encapsulation, it is necessary for the definition of r5l_log to be > >> public so that rcu_access_pointer() can be used on conf-log in the > >> next patch. > >> > >> rcu_access_pointer(p) doesn't technically dereference the log pointer > >> however, it does use typeof(*p) and some older GCC versions (anything > >> earlier than gcc-10) will wrongly try to dereference the structure: > >> > >> include/linux/rcupdate.h:384:9: error: dereferencing pointer to > >> incomplete type ‘struct r5l_log’ > >> > >> typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ > >> ^ > >> > >> include/linux/rcupdate.h:495:31: note: in expansion of > >> macro ‘__rcu_access_pointer’ > >> > >> #define rcu_access_pointer(p) __rcu_access_pointer((p), > >> __UNIQUE_ID(rcu), __rcu) > >> > >> To prevent this, simply provide the definition where > >> rcu_access_pointer() may be used. > > > > What about just moving any code that does the rcu_access_pointer on > > conf->log to raid5-cache.c and doing an out of line call for it > > instead? > > I guess we could do that. All the inline functions in raid5-log.h are > there to choose between the r5l or the ppl implementaiton. So it that > would mean the r5l implementation would probably be inlined and ppl > would be doing a second out of line call. Not sure if that matters, but > it seems a little odd. I like the current version better. raid5-log.h is not used in many files anyway. Thanks, Song
On 2022-06-01 16:36, Song Liu wrote: >> I guess we could do that. All the inline functions in raid5-log.h are >> there to choose between the r5l or the ppl implementaiton. So it that >> would mean the r5l implementation would probably be inlined and ppl >> would be doing a second out of line call. Not sure if that matters, but >> it seems a little odd. > > I like the current version better. raid5-log.h is not used in many files anyway. It's a moot point now. v3 will follow Christoph's other feedback which essentially removes the RCU access altogether and adds an appropriate lock in a couple places. Logan
On Wed, Jun 1, 2022 at 3:42 PM Logan Gunthorpe <logang@deltatee.com> wrote: > > > > On 2022-06-01 16:36, Song Liu wrote: > >> I guess we could do that. All the inline functions in raid5-log.h are > >> there to choose between the r5l or the ppl implementaiton. So it that > >> would mean the r5l implementation would probably be inlined and ppl > >> would be doing a second out of line call. Not sure if that matters, but > >> it seems a little odd. > > > > I like the current version better. raid5-log.h is not used in many files anyway. > > It's a moot point now. v3 will follow Christoph's other feedback which > essentially removes the RCU access altogether and adds an appropriate > lock in a couple places. Ah, that's right. Thanks for pointing it out. Song
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c index a3c4d43d6deb..6349cfaae2c8 100644 --- a/drivers/md/raid5-cache.c +++ b/drivers/md/raid5-cache.c @@ -79,81 +79,6 @@ static char *r5c_journal_mode_str[] = {"write-through", * - return IO for pending writes */ -struct r5l_log { - struct md_rdev *rdev; - - u32 uuid_checksum; - - sector_t device_size; /* log device size, round to - * BLOCK_SECTORS */ - sector_t max_free_space; /* reclaim run if free space is at - * this size */ - - sector_t last_checkpoint; /* log tail. where recovery scan - * starts from */ - u64 last_cp_seq; /* log tail sequence */ - - sector_t log_start; /* log head. where new data appends */ - u64 seq; /* log head sequence */ - - sector_t next_checkpoint; - - struct mutex io_mutex; - struct r5l_io_unit *current_io; /* current io_unit accepting new data */ - - spinlock_t io_list_lock; - struct list_head running_ios; /* io_units which are still running, - * and have not yet been completely - * written to the log */ - struct list_head io_end_ios; /* io_units which have been completely - * written to the log but not yet written - * to the RAID */ - struct list_head flushing_ios; /* io_units which are waiting for log - * cache flush */ - struct list_head finished_ios; /* io_units which settle down in log disk */ - struct bio flush_bio; - - struct list_head no_mem_stripes; /* pending stripes, -ENOMEM */ - - struct kmem_cache *io_kc; - mempool_t io_pool; - struct bio_set bs; - mempool_t meta_pool; - - struct md_thread *reclaim_thread; - unsigned long reclaim_target; /* number of space that need to be - * reclaimed. if it's 0, reclaim spaces - * used by io_units which are in - * IO_UNIT_STRIPE_END state (eg, reclaim - * dones't wait for specific io_unit - * switching to IO_UNIT_STRIPE_END - * state) */ - wait_queue_head_t iounit_wait; - - struct list_head no_space_stripes; /* pending stripes, log has no space */ - spinlock_t no_space_stripes_lock; - - bool need_cache_flush; - - /* for r5c_cache */ - enum r5c_journal_mode r5c_journal_mode; - - /* all stripes in r5cache, in the order of seq at sh->log_start */ - struct list_head stripe_in_journal_list; - - spinlock_t stripe_in_journal_lock; - atomic_t stripe_in_journal_count; - - /* to submit async io_units, to fulfill ordering of flush */ - struct work_struct deferred_io_work; - /* to disable write back during in degraded mode */ - struct work_struct disable_writeback_work; - - /* to for chunk_aligned_read in writeback mode, details below */ - spinlock_t tree_lock; - struct radix_tree_root big_stripe_tree; -}; - /* * Enable chunk_aligned_read() with write back cache. * diff --git a/drivers/md/raid5-log.h b/drivers/md/raid5-log.h index 1c184fe20939..5948c41f1f2e 100644 --- a/drivers/md/raid5-log.h +++ b/drivers/md/raid5-log.h @@ -2,6 +2,81 @@ #ifndef _RAID5_LOG_H #define _RAID5_LOG_H +struct r5l_log { + struct md_rdev *rdev; + + u32 uuid_checksum; + + sector_t device_size; /* log device size, round to + * BLOCK_SECTORS */ + sector_t max_free_space; /* reclaim run if free space is at + * this size */ + + sector_t last_checkpoint; /* log tail. where recovery scan + * starts from */ + u64 last_cp_seq; /* log tail sequence */ + + sector_t log_start; /* log head. where new data appends */ + u64 seq; /* log head sequence */ + + sector_t next_checkpoint; + + struct mutex io_mutex; + struct r5l_io_unit *current_io; /* current io_unit accepting new data */ + + spinlock_t io_list_lock; + struct list_head running_ios; /* io_units which are still running, + * and have not yet been completely + * written to the log */ + struct list_head io_end_ios; /* io_units which have been completely + * written to the log but not yet written + * to the RAID */ + struct list_head flushing_ios; /* io_units which are waiting for log + * cache flush */ + struct list_head finished_ios; /* io_units which settle down in log disk */ + struct bio flush_bio; + + struct list_head no_mem_stripes; /* pending stripes, -ENOMEM */ + + struct kmem_cache *io_kc; + mempool_t io_pool; + struct bio_set bs; + mempool_t meta_pool; + + struct md_thread *reclaim_thread; + unsigned long reclaim_target; /* number of space that need to be + * reclaimed. if it's 0, reclaim spaces + * used by io_units which are in + * IO_UNIT_STRIPE_END state (eg, reclaim + * dones't wait for specific io_unit + * switching to IO_UNIT_STRIPE_END + * state) */ + wait_queue_head_t iounit_wait; + + struct list_head no_space_stripes; /* pending stripes, log has no space */ + spinlock_t no_space_stripes_lock; + + bool need_cache_flush; + + /* for r5c_cache */ + enum r5c_journal_mode r5c_journal_mode; + + /* all stripes in r5cache, in the order of seq at sh->log_start */ + struct list_head stripe_in_journal_list; + + spinlock_t stripe_in_journal_lock; + atomic_t stripe_in_journal_count; + + /* to submit async io_units, to fulfill ordering of flush */ + struct work_struct deferred_io_work; + /* to disable write back during in degraded mode */ + struct work_struct disable_writeback_work; + + /* to for chunk_aligned_read in writeback mode, details below */ + spinlock_t tree_lock; + struct radix_tree_root big_stripe_tree; +}; + int r5l_init_log(struct r5conf *conf, struct md_rdev *rdev); void r5l_exit_log(struct r5conf *conf); int r5l_write_stripe(struct r5conf *conf, struct stripe_head *head_sh);
Move struct r5l_log definition to raid5-log.h. While this reduces encapsulation, it is necessary for the definition of r5l_log to be public so that rcu_access_pointer() can be used on conf-log in the next patch. rcu_access_pointer(p) doesn't technically dereference the log pointer however, it does use typeof(*p) and some older GCC versions (anything earlier than gcc-10) will wrongly try to dereference the structure: include/linux/rcupdate.h:384:9: error: dereferencing pointer to incomplete type ‘struct r5l_log’ typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \ ^ include/linux/rcupdate.h:495:31: note: in expansion of macro ‘__rcu_access_pointer’ #define rcu_access_pointer(p) __rcu_access_pointer((p), __UNIQUE_ID(rcu), __rcu) To prevent this, simply provide the definition where rcu_access_pointer() may be used. Reported-by: Donald Buczek <buczek@molgen.mpg.de> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- drivers/md/raid5-cache.c | 75 ---------------------------------------- drivers/md/raid5-log.h | 75 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 75 deletions(-)