| Message ID | e1b8405de7073547ed6252a314fb467680b4c7e8.1739957534.git.gustavoars@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Avoid a couple hundred -Wflex-array-member-not-at-end warnings | expand |
On Mon, Feb 24, 2025 at 08:27:44PM +1030, Gustavo A. R. Silva wrote: > -Wflex-array-member-not-at-end was introduced in GCC-14, and we are > getting ready to enable it, globally. > > Change the type of the middle struct members currently causing trouble > from `struct bio` to `struct bio_hdr`. > > We also use `container_of()` whenever we need to retrieve a pointer to > the flexible structure `struct bio`, through which we can access the > flexible-array member in it, if necessary. > > With these changes fix 27 of the following warnings: > > fs/xfs/xfs_log_priv.h:208:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > fs/xfs/xfs_log.c | 15 +++++++++------ > fs/xfs/xfs_log_priv.h | 2 +- > 2 files changed, 10 insertions(+), 7 deletions(-) > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index f8851ff835de..7e8b71f64a46 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -1245,7 +1245,7 @@ xlog_ioend_work( > } > > xlog_state_done_syncing(iclog); > - bio_uninit(&iclog->ic_bio); > + bio_uninit(container_of(&iclog->ic_bio, struct bio, __hdr)); > > /* > * Drop the lock to signal that we are done. Nothing references the > @@ -1663,7 +1663,8 @@ xlog_write_iclog( > * writeback throttle from throttling log writes behind background > * metadata writeback and causing priority inversions. > */ > - bio_init(&iclog->ic_bio, log->l_targ->bt_bdev, iclog->ic_bvec, > + bio_init(container_of(&iclog->ic_bio, struct bio, __hdr), > + log->l_targ->bt_bdev, iclog->ic_bvec, > howmany(count, PAGE_SIZE), > REQ_OP_WRITE | REQ_META | REQ_SYNC | REQ_IDLE); > iclog->ic_bio.bi_iter.bi_sector = log->l_logBBstart + bno; > @@ -1692,7 +1693,8 @@ xlog_write_iclog( > > iclog->ic_flags &= ~(XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FUA); > > - if (xlog_map_iclog_data(&iclog->ic_bio, iclog->ic_data, count)) > + if (xlog_map_iclog_data(container_of(&iclog->ic_bio, struct bio, __hdr), > + iclog->ic_data, count)) > goto shutdown; > > if (is_vmalloc_addr(iclog->ic_data)) > @@ -1705,16 +1707,17 @@ xlog_write_iclog( > if (bno + BTOBB(count) > log->l_logBBsize) { > struct bio *split; > > - split = bio_split(&iclog->ic_bio, log->l_logBBsize - bno, > + split = bio_split(container_of(&iclog->ic_bio, struct bio, __hdr), > + log->l_logBBsize - bno, > GFP_NOIO, &fs_bio_set); > - bio_chain(split, &iclog->ic_bio); > + bio_chain(split, container_of(&iclog->ic_bio, struct bio, __hdr)); > submit_bio(split); > > /* restart at logical offset zero for the remainder */ > iclog->ic_bio.bi_iter.bi_sector = log->l_logBBstart; > } > > - submit_bio(&iclog->ic_bio); > + submit_bio(container_of(&iclog->ic_bio, struct bio, __hdr)); > return; > shutdown: > xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR); > diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h > index f3d78869e5e5..32abc48aef24 100644 > --- a/fs/xfs/xfs_log_priv.h > +++ b/fs/xfs/xfs_log_priv.h > @@ -205,7 +205,7 @@ typedef struct xlog_in_core { > #endif > struct semaphore ic_sema; > struct work_struct ic_end_io_work; > - struct bio ic_bio; > + struct bio_hdr ic_bio; What struct is this? $ git grep 'struct bio_hdr' include/ $ (Please always send the core code change patches to the xfs list.) --D > struct bio_vec ic_bvec[]; > } xlog_in_core_t; > > -- > 2.43.0 > >
On Mon, Feb 24, 2025 at 08:27:44PM +1030, Gustavo A. R. Silva wrote: > -Wflex-array-member-not-at-end was introduced in GCC-14, and we are > getting ready to enable it, globally. > > Change the type of the middle struct members currently causing trouble > from `struct bio` to `struct bio_hdr`. What's this bio_hdr thing? You haven't sent the patch to the XFS list, so we cannot review this for correctness. Please cc the -entire- patch set to -all- recipients so we can see the whole change in it's full context. > We also use `container_of()` whenever we need to retrieve a pointer to > the flexible structure `struct bio`, through which we can access the > flexible-array member in it, if necessary. > > With these changes fix 27 of the following warnings: > > fs/xfs/xfs_log_priv.h:208:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > fs/xfs/xfs_log.c | 15 +++++++++------ > fs/xfs/xfs_log_priv.h | 2 +- > 2 files changed, 10 insertions(+), 7 deletions(-) > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index f8851ff835de..7e8b71f64a46 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -1245,7 +1245,7 @@ xlog_ioend_work( > } > > xlog_state_done_syncing(iclog); > - bio_uninit(&iclog->ic_bio); > + bio_uninit(container_of(&iclog->ic_bio, struct bio, __hdr)); This is a pretty nasty conversion. The code is obviously correct right now - the bio uses an external bvec table, so has no associated allocated bvec space and so the bio isn't flexibly sized. Making the code more complex for humans to understand and get right because "the compiler is dumb" is a bad tradeoff. I also think this is a really nasty way of fixing the problem; casting the fixed size structure to a flex sized structure that can overlap other parent structure fields really isn't a good solution. IMO, it is a recipe for unexpected memory corruption when the bio isn't set up properly by the caller or there are bugs in the way the bio is iterated/processed.... > return; > shutdown: > xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR); > diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h > index f3d78869e5e5..32abc48aef24 100644 > --- a/fs/xfs/xfs_log_priv.h > +++ b/fs/xfs/xfs_log_priv.h > @@ -205,7 +205,7 @@ typedef struct xlog_in_core { > #endif > struct semaphore ic_sema; > struct work_struct ic_end_io_work; > - struct bio ic_bio; > + struct bio_hdr ic_bio; > struct bio_vec ic_bvec[]; But then there is the bigger question: ic_bvec is a *fixed size* that is allocated at mount time. The fact it is defined as a flexible array is the problem here, not the embedding of a struct bio. i.e. we've used a flex array to optimise away an extra allocation in a non-performance sensitive path. Hence if we had done it this way: struct bvec *ic_bvecs; struct bio ic_bio; // must be last }; and use another allocation for ic_bvecs in xlog_alloc_log() (similar to how we do the separate allocation of the *ic_data buffer that is mapped into ic_bio at IO time), then none of the code that accesses ic_bio for IO purposes needs to change and the compiler warnings go away. That's a much cleaner solution that requires no extra cognitive load on developers to maintain in correct working order. -Dave.
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index f8851ff835de..7e8b71f64a46 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -1245,7 +1245,7 @@ xlog_ioend_work( } xlog_state_done_syncing(iclog); - bio_uninit(&iclog->ic_bio); + bio_uninit(container_of(&iclog->ic_bio, struct bio, __hdr)); /* * Drop the lock to signal that we are done. Nothing references the @@ -1663,7 +1663,8 @@ xlog_write_iclog( * writeback throttle from throttling log writes behind background * metadata writeback and causing priority inversions. */ - bio_init(&iclog->ic_bio, log->l_targ->bt_bdev, iclog->ic_bvec, + bio_init(container_of(&iclog->ic_bio, struct bio, __hdr), + log->l_targ->bt_bdev, iclog->ic_bvec, howmany(count, PAGE_SIZE), REQ_OP_WRITE | REQ_META | REQ_SYNC | REQ_IDLE); iclog->ic_bio.bi_iter.bi_sector = log->l_logBBstart + bno; @@ -1692,7 +1693,8 @@ xlog_write_iclog( iclog->ic_flags &= ~(XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FUA); - if (xlog_map_iclog_data(&iclog->ic_bio, iclog->ic_data, count)) + if (xlog_map_iclog_data(container_of(&iclog->ic_bio, struct bio, __hdr), + iclog->ic_data, count)) goto shutdown; if (is_vmalloc_addr(iclog->ic_data)) @@ -1705,16 +1707,17 @@ xlog_write_iclog( if (bno + BTOBB(count) > log->l_logBBsize) { struct bio *split; - split = bio_split(&iclog->ic_bio, log->l_logBBsize - bno, + split = bio_split(container_of(&iclog->ic_bio, struct bio, __hdr), + log->l_logBBsize - bno, GFP_NOIO, &fs_bio_set); - bio_chain(split, &iclog->ic_bio); + bio_chain(split, container_of(&iclog->ic_bio, struct bio, __hdr)); submit_bio(split); /* restart at logical offset zero for the remainder */ iclog->ic_bio.bi_iter.bi_sector = log->l_logBBstart; } - submit_bio(&iclog->ic_bio); + submit_bio(container_of(&iclog->ic_bio, struct bio, __hdr)); return; shutdown: xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR); diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h index f3d78869e5e5..32abc48aef24 100644 --- a/fs/xfs/xfs_log_priv.h +++ b/fs/xfs/xfs_log_priv.h @@ -205,7 +205,7 @@ typedef struct xlog_in_core { #endif struct semaphore ic_sema; struct work_struct ic_end_io_work; - struct bio ic_bio; + struct bio_hdr ic_bio; struct bio_vec ic_bvec[]; } xlog_in_core_t;
-Wflex-array-member-not-at-end was introduced in GCC-14, and we are getting ready to enable it, globally. Change the type of the middle struct members currently causing trouble from `struct bio` to `struct bio_hdr`. We also use `container_of()` whenever we need to retrieve a pointer to the flexible structure `struct bio`, through which we can access the flexible-array member in it, if necessary. With these changes fix 27 of the following warnings: fs/xfs/xfs_log_priv.h:208:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- fs/xfs/xfs_log.c | 15 +++++++++------ fs/xfs/xfs_log_priv.h | 2 +- 2 files changed, 10 insertions(+), 7 deletions(-)