Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Avri Altman <Avri.Altman@wdc.com>
To:     Jiaming Li <lijiamingsofine@gmail.com>,
        "alim.akhtar@samsung.com" <alim.akhtar@samsung.com>,
        "jejb@linux.ibm.com" <jejb@linux.ibm.com>
CC:     "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        lijiaming3 <lijiaming3@xiaomi.com>
Subject: RE: [PATCH] scsi: ufs: ufsfbo: Introduce File Based Optimization
 feature
Thread-Topic: [PATCH] scsi: ufs: ufsfbo: Introduce File Based Optimization
 feature
Thread-Index: AQHYt5Yd7MdiBMU/V0WwXjppS39zs62/PhVw
Date:   Thu, 25 Aug 2022 08:42:38 +0000
Message-ID: <DM6PR04MB657508E997AD3AAE5BB445ACFC729@DM6PR04MB6575.namprd04.prod.outlook.com>
References: <20220824084633.14428-1-lijiamingsofine@gmail.com>
In-Reply-To: <20220824084633.14428-1-lijiamingsofine@gmail.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?HfJtwL1xXacNHqR3MVULPoQOecPvfEApuLsHyPvdb4dtWkjwifEj3hz9h16P?=
 =?us-ascii?Q?WvTrn4DrprC+dzwcrI6AYnw0jWfYXUpIKju9hHWaVn5nQzrvZDeflpiB7t99?=
 =?us-ascii?Q?Z7xOe5i5LeDkeNURFAqyMvt62m1lPLKra98NRQJI1ql6MZMDN2CtS3zNbORP?=
 =?us-ascii?Q?pmm8SK8dOmaV5sYi7piXjo8fujdx1uiO3CZ07kOvv0T10HFv8Dc44yu5wQkg?=
 =?us-ascii?Q?c8w5cxwqQqvPujwQb5loGkG1MpRzKKlXU3dkZFJdZmvqoA0gUqi+vX9YQf/e?=
 =?us-ascii?Q?mIHKfi3ImATjunRENOskFMsRZFJi5NCH8ICTl7buGuYHkXKwqmyLSFpUiOPx?=
 =?us-ascii?Q?0eBzPyofFfAWfmQYjsJ9rn29PPmxPe8wcrBP2mQ9HxNYIFLA39Vge7SsgVi7?=
 =?us-ascii?Q?MAObsOKNuIqOtj+3Op0HZBaLLcqByvcFHnlYiox2ieDGPUr6Z+bM4iG9KWV4?=
 =?us-ascii?Q?GepcDde90MQHteXlnZk28gJhzaRSuNUmsAyyv/NQJ8DnecW53JmZllaOKrgk?=
 =?us-ascii?Q?WD3nVCTqN8XYwMnlSAW6nwXsC/upOCp6Bb6AYmv/SmExvzmZpYXdkqPMj9EG?=
 =?us-ascii?Q?6w+TTZnYaHHnQ4EYPelk8NNvh/H6WthTLCM7BkVo0R2sbt2+BS1Y87MuFQ4j?=
 =?us-ascii?Q?xKmYK3BRlc+I2EHi0nNtkaO68QVMLudclV93Jcq+SW19skHniEWzTfjRVAkq?=
 =?us-ascii?Q?zmfqhtPJZYNJiVMIEDbYQ3TfpmC2NeVHU1SAJn1rylz8t5ipdjBN1jLaqNp4?=
 =?us-ascii?Q?qXStVX0QHYV8dxcUBXBsNlleR9FPn3NDmXASaPFP52kOfjApwYENYFryfmvI?=
 =?us-ascii?Q?ZNH40PTNdsRrx4jHkCSUaXIMwrD2smNDG616M8UIjLIXxomBZzPhQXvsNImf?=
 =?us-ascii?Q?jOAGpl2XencnqKTj8tI9bcZX+iTS3H+t2X30jUugUFbCBVf0HqsnRhPAjrAB?=
 =?us-ascii?Q?X6ktusl3qLLM36qQnWmFnh2qUvsKHf7AiMQx40vbAtXY/LIchdV6Ypu2wCeU?=
 =?us-ascii?Q?AqjDnbrGaQx8rAz+R/jyy63vF1FzcmpPT4qq+Ongz5p5HxjYant2Twmzxeqi?=
 =?us-ascii?Q?94GydWc8tPtRZ/Tmt9VkIHKlO6hrOCSvJ63iGBUt3fJntnFxMqB53m6I6XPh?=
 =?us-ascii?Q?JjSreTCRW7dQRYsquUN7QiWrQA3QV0n3dqMGq6GlqJBRqS2unhw0GLMolYcq?=
 =?us-ascii?Q?UcFE+jVnxMl4Jq6c8SJSiHARinMPydv8f+m1lBy0LqA3MHLuGoTgPEjVM3vC?=
 =?us-ascii?Q?bgSSlbMa7vhwq2W4TbfP4H5WeGK4vG/WJ+gCE/wHkap+WCBhTBGsC25QtIsg?=
 =?us-ascii?Q?88KFGWgGARTibxAKild9twQ7OXNy6FjqZlYPwdW4qH6X4e9FfSfZEw0QyMrj?=
 =?us-ascii?Q?8W3Co5HDDR/kP2hzDiI90nMIiRjoiTB9lODi8rEIUpvoH0t//2e8t2PP8aa3?=
 =?us-ascii?Q?z2Q17JoftsJxwfEr75dz2D3qlzq5QBwvgL+OuSnfe9d2Cda2A+LFt7xq8GIV?=
 =?us-ascii?Q?PbtZmXA9nUHRRc+dgOjHafetqLuuS6WomiUSzcO2NY+kj5r8CdybfsvncIMB?=
 =?us-ascii?Q?LQTbts3WVSwdJErukf63RaXsvGeIEP5moZOdqI4a?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: wdc.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR04MB6575.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 64d478f4-2a81-4346-2c6d-08da8675c417
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Aug 2022 08:42:38.5182
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: OSxeIG6gJyYxrtzuT8Q3CMVWBCNxS7uhQVtN0fV5+nH6CW1Nrx/qvmZ5do08uSgioo2BMxp6JaXkhltobSLM2A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR04MB6205
X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,

> From: lijiaming3 <lijiaming3@xiaomi.com>
>=20
> Implement File Based Optimization initialization and add sysfs interface.
>=20
> Stoage devices have a long lifespan. Device performance over its lifespan=
 is not
> constant and may deteriorate over time
>=20
> This feature describes a method to improve the performance regression.
> The host needs to provide File System information to storage device first=
. Based
> on that information device analyzes the file system data and provides the=
 host
> the level of performance regression. The host then may instruct the devic=
e to
> execute optimization procedure to improve the regression level.
You might want to break this one long patch into a series of smaller patche=
s.
One option is to do it per-functional flows: Init flow, analysis phase, and=
 defrag phase.
In your cover letter you might want to elaborate what this feature is all a=
bout,
And that your design expect a user-space companion that analyze the files.
Which is a good place to share a reference to an open source code that does=
 that.


> +void ufsfbo_init(struct ufs_hba *hba)
Maybe ufshbo_probe to note that you are probing the device for this feature=
?

> +{
> +       struct ufsfbo_dev *fbo;
> +       int ret =3D 0;
> +
> +       fbo =3D &hba->fbo;
> +       fbo->hba =3D hba;
> +
> +       ret =3D ufsfbo_get_fbo_support_state(fbo);
You can save reading the device descriptor, if you would call ufsfbo_init f=
rom ufs_get_device_desc,
Like ufshcd_wb_probe() does.


> +       if (ret) {
> +               FBO_ERR_MSG("NOT Support FBO. ret(%d)", ret);
> +               return;
> +       }
> +       ret =3D ufsfbo_get_fbo_desc_info(fbo);
> +       if (ret) {
> +               FBO_ERR_MSG("Failed getting fbo info. ret(%d)", ret);
> +               return;
> +       }
> +       fbo->fbo_debug =3D false;
> +       fbo->block_suspend =3D false;
> +
> +       if (ufshcd_is_auto_hibern8_supported(fbo->hba))
> +               fbo->is_auto_enabled =3D true;
All this power management control code is not needed IMO.

> +
> +       ret =3D ufsfbo_create_sysfs(fbo);
You might want to consider splitting your sysfs according to:
 - FBOWritebuffer & FBOReadbuffer - part of sdev_group
   it will allow defragmenting files across all luns.
 - the rest per hba


> +static ssize_t ufsfbo_sysfs_show_debug(struct ufsfbo_dev *fbo, char
> +*buf) {
> +       FBO_INFO_MSG(fbo, "Debug:%d", fbo->fbo_debug);
> +       return sysfs_emit(buf, "%d\n", fbo->fbo_debug); }
I don't think the debug info should be part of your series.
Maybe add it as the last patch if you must.


> +/**
> + * struct ufsfbo_dev - FBO related structure
Maybe ufsfbo_ctrl - as it's your main controlling object.
And split it further to ufsfbo_dev_info - ver, rec_lrs, max_lrs, etc.
And then all other members

> +int ufsfbo_get_fbo_prog_state(struct ufsfbo_dev *fbo, int *prog_state);
> +int ufsfbo_operation_control(struct ufsfbo_dev *fbo, int *val); int
> +ufsfbo_get_exe_level(struct ufsfbo_dev *fbo, int *frag_exe_level); int
> +ufsfbo_set_exe_level(struct ufsfbo_dev *fbo, int *frag_exe_level); int
> +ufsfbo_lba_list_write(struct ufsfbo_dev *fbo, const char *buf); int
> +ufsfbo_read_frag_level(struct ufsfbo_dev *fbo, char *buf); int
> +ufsfbo_get_fbo_desc_info(struct ufsfbo_dev *fbo); int
> +ufsfbo_get_fbo_support_state(struct ufsfbo_dev *fbo); int
> +ufsfbo_get_state(struct ufs_hba *hba); void ufsfbo_set_state(struct
> +ufs_hba *hba, int state); void ufsfbo_init(struct ufs_hba *hba); void
> +ufsfbo_reset(struct ufs_hba *hba); void ufsfbo_reset_host(struct
> +ufs_hba *hba); void ufsfbo_remove(struct ufs_hba *hba); int
> +ufsfbo_is_not_present(struct ufsfbo_dev *fbo); void
> +ufsfbo_block_enter_suspend(struct ufsfbo_dev *fbo); void
> +ufsfbo_allow_enter_suspend(struct ufsfbo_dev *fbo); void
> +ufsfbo_auto_hibern8_enable(struct ufsfbo_dev *fbo, unsigned int val);
> +#endif /* _UFSFBO_H_ */
You should expose functionality outside the module only if you must.
Otherwise, make it privet to the module.

> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 2f6468f22b48..9c377c514e17 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4956,6 +4956,14 @@ static void ufshcd_hpb_configure(struct ufs_hba
> *hba, struct scsi_device *sdev)
>         ufshpb_init_hpb_lu(hba, sdev);
>  }
>=20
> +static void ufshcd_fbo_configure(struct ufs_hba *hba, struct
> +scsi_device *sdev) { #ifdef CONFIG_SCSI_UFS_FBO
> +       if (sdev->lun =3D=3D 0)
> +               hba->fbo.sdev_ufs_lu =3D sdev; #endif }
You won't be needing this if you follow my suggestion above.


> @@ -8005,6 +8014,9 @@ static void ufshcd_async_scan(void *data,
> async_cookie_t cookie)
>=20
>         /* Probe and add UFS logical units  */
>         ret =3D ufshcd_add_lus(hba);
> +#ifdef CONFIG_SCSI_UFS_FBO
> +       ufsfbo_init(hba);
> +#endif
Instead of wrapping it withing other modules,
Use it in ufsfbo.h

> +#ifdef CONFIG_SCSI_UFS_FBO
> +#include "ufsfbo.h"
> +#endif
Include it in ufshcd.c without the ifdef

>  #define UFSHCD "ufshcd"
>  #define UFSHCD_DRIVER_VERSION "0.2"
>=20
> @@ -916,6 +918,9 @@ struct ufs_hba {
>         struct dentry *debugfs_root;
>         struct delayed_work debugfs_ee_work;
>         u32 debugfs_ee_rate_limit_ms;
> +#endif
> +#ifdef CONFIG_SCSI_UFS_FBO
> +       struct ufsfbo_dev fbo;
>  #endif
If you would use a struct pointer here, you won't need the ifdef.

Thanks,
Avri