Date:   Thu, 25 Aug 2022 08:42:38 +0000
From:   Avri Altman <Avri.Altman@....com>
To:     Jiaming Li <lijiamingsofine@...il.com>,
        "alim.akhtar@...sung.com" <alim.akhtar@...sung.com>,
        "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>
CC:     "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        lijiaming3 <lijiaming3@...omi.com>
Subject: RE: [PATCH] scsi: ufs: ufsfbo: Introduce File Based Optimization
 feature

Hi,

> From: lijiaming3 <lijiaming3@...omi.com>
> 
> Implement File Based Optimization initialization and add sysfs interface.
> 
> Stoage devices have a long lifespan. Device performance over its lifespan is not
> constant and may deteriorate over time
> 
> 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 device to
> execute optimization procedure to improve the regression level.
You might want to break this one long patch into a series of smaller patches.
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 about,
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 = 0;
> +
> +       fbo = &hba->fbo;
> +       fbo->hba = hba;
> +
> +       ret = ufsfbo_get_fbo_support_state(fbo);
You can save reading the device descriptor, if you would call ufsfbo_init from ufs_get_device_desc,
Like ufshcd_wb_probe() does.


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

> +
> +       ret = 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);
>  }
> 
> +static void ufshcd_fbo_configure(struct ufs_hba *hba, struct
> +scsi_device *sdev) { #ifdef CONFIG_SCSI_UFS_FBO
> +       if (sdev->lun == 0)
> +               hba->fbo.sdev_ufs_lu = 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)
> 
>         /* Probe and add UFS logical units  */
>         ret = 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"
> 
> @@ -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

Powered by blists - more mailing lists