| Message ID | 20190813090306.31278-4-nborisov@suse.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | Minor cleanups | expand |
On Tue, Aug 13, 2019 at 12:03:06PM +0300, Nikolay Borisov wrote: > This macro encodes a trivial struct initializations, just open code it. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- Seems fine: Reviewed-by: Brian Foster <bfoster@redhat.com> What might be more interesting is to audit the cases where nmap is always 1 and see if we can start eliminating some of this code where it isn't needed. For example, it looks xfs_buf_readahead_map() only ever uses nmap == 1. Can we pass a block/len directly there and push the map/nmap parameters further down the stack? FWIW, I also see several functions on a quick glance (xfs_dabuf_map(), xfs_buf_map_from_irec()) that take map/nmaps params, assert that nmaps == 1 yet still have iteration code for nmap > 1 cases. > fs/xfs/xfs_buf.c | 4 ++-- > fs/xfs/xfs_buf.h | 9 +++------ > fs/xfs/xfs_trans.h | 6 ++++-- > 3 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 99c66f80d7cc..389c5b590f11 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -658,7 +658,7 @@ xfs_buf_incore( > { > struct xfs_buf *bp; > int error; > - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); > + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; > > error = xfs_buf_find(target, &map, 1, flags, NULL, &bp); > if (error) > @@ -905,7 +905,7 @@ xfs_buf_get_uncached( > unsigned long page_count; > int error, i; > struct xfs_buf *bp; > - DEFINE_SINGLE_BUF_MAP(map, XFS_BUF_DADDR_NULL, numblks); > + struct xfs_buf_map map = { .bm_bn = XFS_BUF_DADDR_NULL, .bm_len = numblks }; > > /* flags might contain irrelevant bits, pass only what we care about */ > bp = _xfs_buf_alloc(target, &map, 1, flags & XBF_NO_IOACCT); > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h > index ec7037284d62..548dfb0c6e27 100644 > --- a/fs/xfs/xfs_buf.h > +++ b/fs/xfs/xfs_buf.h > @@ -104,9 +104,6 @@ struct xfs_buf_map { > int bm_len; /* size of I/O */ > }; > > -#define DEFINE_SINGLE_BUF_MAP(map, blkno, numblk) \ > - struct xfs_buf_map (map) = { .bm_bn = (blkno), .bm_len = (numblk) }; > - > struct xfs_buf_ops { > char *name; > union { > @@ -209,7 +206,7 @@ xfs_buf_get( > xfs_daddr_t blkno, > size_t numblks) > { > - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); > + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; > return xfs_buf_get_map(target, &map, 1, 0); > } > > @@ -221,7 +218,7 @@ xfs_buf_read( > xfs_buf_flags_t flags, > const struct xfs_buf_ops *ops) > { > - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); > + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; > return xfs_buf_read_map(target, &map, 1, flags, ops); > } > > @@ -232,7 +229,7 @@ xfs_buf_readahead( > size_t numblks, > const struct xfs_buf_ops *ops) > { > - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); > + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; > return xfs_buf_readahead_map(target, &map, 1, ops); > } > > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h > index 64d7f171ebd3..8d6fce5c0320 100644 > --- a/fs/xfs/xfs_trans.h > +++ b/fs/xfs/xfs_trans.h > @@ -182,7 +182,8 @@ xfs_trans_get_buf( > int numblks, > uint flags) > { > - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); > + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; > + > return xfs_trans_get_buf_map(tp, target, &map, 1, flags); > } > > @@ -205,7 +206,8 @@ xfs_trans_read_buf( > struct xfs_buf **bpp, > const struct xfs_buf_ops *ops) > { > - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); > + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; > + > return xfs_trans_read_buf_map(mp, tp, target, &map, 1, > flags, bpp, ops); > } > -- > 2.17.1 >
On Tue, Aug 13, 2019 at 12:03:06PM +0300, Nikolay Borisov wrote: > This macro encodes a trivial struct initializations, just open code it. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> Hmmmm. We have defines for this sort of structure definition and initialisation all over the kernel. e.g. LIST_HEAD(), DEFINE_PER_CPU(), DEFINE_HASHTABLE(), DEFINE_SPINLOCK(), etc... And really, the intent of the define was to make it easy to get rid of all the callers of the non-map buffer interfaces by moving the map definition into the callers of xfs_buf_get, _read, etc and then enabling use to remove the non-map interfaces altogether. Hence I'd much prefer to see the xfs_buf_{get,read,readahead} and xfs_trans_buf_{get,read} wrapper functions go away than removing the define. > --- > fs/xfs/xfs_buf.c | 4 ++-- > fs/xfs/xfs_buf.h | 9 +++------ > fs/xfs/xfs_trans.h | 6 ++++-- > 3 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 99c66f80d7cc..389c5b590f11 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -658,7 +658,7 @@ xfs_buf_incore( > { > struct xfs_buf *bp; > int error; > - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); > + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; FWIW, I'm not a fan of single line definitions like this because they are really hard to read. If you are converting to this form, it should be like this: struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks, }; Cheers, Dave.
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 99c66f80d7cc..389c5b590f11 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -658,7 +658,7 @@ xfs_buf_incore( { struct xfs_buf *bp; int error; - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; error = xfs_buf_find(target, &map, 1, flags, NULL, &bp); if (error) @@ -905,7 +905,7 @@ xfs_buf_get_uncached( unsigned long page_count; int error, i; struct xfs_buf *bp; - DEFINE_SINGLE_BUF_MAP(map, XFS_BUF_DADDR_NULL, numblks); + struct xfs_buf_map map = { .bm_bn = XFS_BUF_DADDR_NULL, .bm_len = numblks }; /* flags might contain irrelevant bits, pass only what we care about */ bp = _xfs_buf_alloc(target, &map, 1, flags & XBF_NO_IOACCT); diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h index ec7037284d62..548dfb0c6e27 100644 --- a/fs/xfs/xfs_buf.h +++ b/fs/xfs/xfs_buf.h @@ -104,9 +104,6 @@ struct xfs_buf_map { int bm_len; /* size of I/O */ }; -#define DEFINE_SINGLE_BUF_MAP(map, blkno, numblk) \ - struct xfs_buf_map (map) = { .bm_bn = (blkno), .bm_len = (numblk) }; - struct xfs_buf_ops { char *name; union { @@ -209,7 +206,7 @@ xfs_buf_get( xfs_daddr_t blkno, size_t numblks) { - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; return xfs_buf_get_map(target, &map, 1, 0); } @@ -221,7 +218,7 @@ xfs_buf_read( xfs_buf_flags_t flags, const struct xfs_buf_ops *ops) { - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; return xfs_buf_read_map(target, &map, 1, flags, ops); } @@ -232,7 +229,7 @@ xfs_buf_readahead( size_t numblks, const struct xfs_buf_ops *ops) { - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; return xfs_buf_readahead_map(target, &map, 1, ops); } diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h index 64d7f171ebd3..8d6fce5c0320 100644 --- a/fs/xfs/xfs_trans.h +++ b/fs/xfs/xfs_trans.h @@ -182,7 +182,8 @@ xfs_trans_get_buf( int numblks, uint flags) { - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; + return xfs_trans_get_buf_map(tp, target, &map, 1, flags); } @@ -205,7 +206,8 @@ xfs_trans_read_buf( struct xfs_buf **bpp, const struct xfs_buf_ops *ops) { - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; + return xfs_trans_read_buf_map(mp, tp, target, &map, 1, flags, bpp, ops); }
This macro encodes a trivial struct initializations, just open code it. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/xfs/xfs_buf.c | 4 ++-- fs/xfs/xfs_buf.h | 9 +++------ fs/xfs/xfs_trans.h | 6 ++++-- 3 files changed, 9 insertions(+), 10 deletions(-)