[lustre-devel] [PATCH 11/30] lustre: mdc: allow setting readdir RPC size parameter
Andreas Dilger
adilger at whamcloud.com
Wed Sep 19 22:42:09 PDT 2018
On Sep 18, 2018, at 09:14, NeilBrown <neilb at suse.com> wrote:
>
> On Mon, Sep 17 2018, James Simmons wrote:
>
>> From: Andreas Dilger <adilger at whamcloud.com>
>>
>> diff --git a/drivers/staging/lustre/lustre/include/lustre_net.h b/drivers/staging/lustre/lustre/include/lustre_net.h
>> index 2dbd208..cf630db 100644
>> --- a/drivers/staging/lustre/lustre/include/lustre_net.h
>> +++ b/drivers/staging/lustre/lustre/include/lustre_net.h
>> @@ -104,15 +104,15 @@
>> * currently supported maximum between peers at connect via ocd_brw_size.
>> */
>> #define PTLRPC_MAX_BRW_BITS (LNET_MTU_BITS + PTLRPC_BULK_OPS_BITS)
>> -#define PTLRPC_MAX_BRW_SIZE (1 << PTLRPC_MAX_BRW_BITS)
>> +#define PTLRPC_MAX_BRW_SIZE BIT(PTLRPC_MAX_BRW_BITS)
>> #define PTLRPC_MAX_BRW_PAGES (PTLRPC_MAX_BRW_SIZE >> PAGE_SHIFT)
>>
>> -#define ONE_MB_BRW_SIZE (1 << LNET_MTU_BITS)
>> -#define MD_MAX_BRW_SIZE (1 << LNET_MTU_BITS)
>> +#define ONE_MB_BRW_SIZE BIT(LNET_MTU_BITS)
>> +#define MD_MAX_BRW_SIZE BIT(LNET_MTU_BITS)
>> #define MD_MAX_BRW_PAGES (MD_MAX_BRW_SIZE >> PAGE_SHIFT)
>> #define DT_MAX_BRW_SIZE PTLRPC_MAX_BRW_SIZE
>> #define DT_MAX_BRW_PAGES (DT_MAX_BRW_SIZE >> PAGE_SHIFT)
>> -#define OFD_MAX_BRW_SIZE (1 << LNET_MTU_BITS)
>> +#define OFD_MAX_BRW_SIZE BIT(LNET_MTU_BITS)
>
> Argg!! No!! Names are important.
> "SIZE" means the value is a size, a number. The bit-representation is
> largely irrelevant, it is the number that is important.
> BIT(x) returns a single bit - lots of zeros and just one '1' bit. This
> is not a number, it is a bit pattern.
>
> So settings FOO_SIZE to BIT(bar) is wrong. It is a type error. It uses
> a bit pattern when a number is expected. The C compiler won't notice, but I will.
>
> When I apply this (which probably won't be until next week), I'll just
> remove this section of the patch.
Just to confirm, my original patch didn't have these BIT() macros in it,
and I agree with your statements, so I'm fine with you removing them again.
>> diff --git a/drivers/staging/lustre/lustre/llite/lcommon_cl.c b/drivers/staging/lustre/lustre/llite/lcommon_cl.c
>> index 6c9fe49..d3b0445 100644
>> --- a/drivers/staging/lustre/lustre/llite/lcommon_cl.c
>> +++ b/drivers/staging/lustre/lustre/llite/lcommon_cl.c
>> @@ -267,27 +267,22 @@ void cl_inode_fini(struct inode *inode)
>> /**
>> * build inode number from passed @fid
>> */
>> -__u64 cl_fid_build_ino(const struct lu_fid *fid, int api32)
>> +u64 cl_fid_build_ino(const struct lu_fid *fid, int api32)
>> {
>> if (BITS_PER_LONG == 32 || api32)
>> return fid_flatten32(fid);
>> - else
>> - return fid_flatten(fid);
>> +
>> + return fid_flatten(fid);
>
> I preferred that as it was - it makes the either/or nature more obvious.
Kernel style generally recommends no "else" after a return, and checkpatch.pl will complain in this case.
Cheers, Andreas
---
Andreas Dilger
Principal Lustre Architect
Whamcloud
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 235 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20180920/8d434fe1/attachment.sig>
More information about the lustre-devel
mailing list