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 <[email protected]>
Signed-off-by: Logan Gunthorpe <[email protected]>
---
drivers/md/raid5-cache.c | 75 ----------------------------------------
drivers/md/raid5-log.h | 75 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 75 insertions(+), 75 deletions(-)
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);
--
2.30.2
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 <[email protected]> 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 <[email protected]> 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