Received-SPF: pass (google.com: domain of linux-kernel+bounces-145916-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <207dd4f9-f399-40db-b91e-df649f2c52d2@molgen.mpg.de>
Date: Mon, 15 Apr 2024 23:14:10 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] md: Added journal count to md_u.h
To: Abhinav Jain <jain.abhinav177@gmail.com>
Cc: song@kernel.org, yukuai3@huawei.com, linux-raid@vger.kernel.org,
 linux-kernel@vger.kernel.org, skhan@linuxfoundation.org,
 javier.carrasco.cruz@gmail.com
References: <20240415160818.8276-1-jain.abhinav177@gmail.com>
Content-Language: en-US
From: Paul Menzel <pmenzel@molgen.mpg.de>
In-Reply-To: <20240415160818.8276-1-jain.abhinav177@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Dear Abhinav,


Thank you for your patch. Some formal nits. Could you please use 
imperative mood in the commit message summary/title?

Am 15.04.24 um 18:08 schrieb Abhinav Jain:
> This patch addresses TODO in "./drivers/md/md.c:6752".

Maybe quote the to-do?

> Add journal struct member to mdu_array_info_t in "md_u.h".
> Add the journal field in get_array_info function in "md.c".

I wouldn’t break the line, just because a sentence ends, and use 75 
characters per line. Or make it a list/enumeration.

> Need feedback on if the update_array_info function needs to be updated.

If you need feedback, please tag the patch, that means, add [RFC] to the 
subject/summary/title.

How did you test this?


Kind regards,

Paul


> Signed-off-by: Abhinav Jain <jain.abhinav177@gmail.com>
> ---
>   drivers/md/md.c                | 8 ++++----
>   include/uapi/linux/raid/md_u.h | 1 +
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index e575e74aabf5..4958d6e21923 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -6735,10 +6735,10 @@ static int get_version(void __user *arg)
>   static int get_array_info(struct mddev *mddev, void __user *arg)
>   {
>   	mdu_array_info_t info;
> -	int nr,working,insync,failed,spare;
> +	int nr, working, insync, failed, spare, journal;
>   	struct md_rdev *rdev;
>   
> -	nr = working = insync = failed = spare = 0;
> +	nr = working = insync = failed = spare = journal = 0;
>   	rcu_read_lock();
>   	rdev_for_each_rcu(rdev, mddev) {
>   		nr++;
> @@ -6749,8 +6749,7 @@ static int get_array_info(struct mddev *mddev, void __user *arg)
>   			if (test_bit(In_sync, &rdev->flags))
>   				insync++;
>   			else if (test_bit(Journal, &rdev->flags))
> -				/* TODO: add journal count to md_u.h */
> -				;
> +				journal++;
>   			else
>   				spare++;
>   		}
> @@ -6769,6 +6768,7 @@ static int get_array_info(struct mddev *mddev, void __user *arg)
>   	info.raid_disks    = mddev->raid_disks;
>   	info.md_minor      = mddev->md_minor;
>   	info.not_persistent= !mddev->persistent;
> +	info.journal       = journal;
>   
>   	info.utime         = clamp_t(time64_t, mddev->utime, 0, U32_MAX);
>   	info.state         = 0;
> diff --git a/include/uapi/linux/raid/md_u.h b/include/uapi/linux/raid/md_u.h
> index 7be89a4906e7..485befa2205d 100644
> --- a/include/uapi/linux/raid/md_u.h
> +++ b/include/uapi/linux/raid/md_u.h
> @@ -84,6 +84,7 @@ typedef struct mdu_array_info_s {
>   	int raid_disks;
>   	int md_minor;
>   	int not_persistent;
> +	int journal;
>   
>   	/*
>   	 * Generic state information