lkml.org 
[lkml]   [2022]   [Aug]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH] scsi: ufs: ufsfbo: Introduce File Based Optimization feature
    Date
    Hi,

    > From: lijiaming3 <lijiaming3@xiaomi.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

    \
     
     \ /
      Last update: 2022-08-25 10:43    [W:4.554 / U:1.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site