On Mon, Mar 15, 2021 at 08:39:31PM +0800, Qu Wenruo wrote:
>
>
> On 2021/3/15 下午7:59, Anand Jain wrote:
> > On 10/03/2021 17:08, Qu Wenruo wrote:
> >> Add extra sysfs interface features/supported_ro_sectorsize and
> >> features/supported_rw_sectorsize to indicate subpage support.
> >>
> >> Currently for supported_rw_sectorsize all architectures only have their
> >> PAGE_SIZE listed.
> >>
> >> While for supported_ro_sectorsize, for systems with 64K page size, 4K
> >> sectorsize is also supported.
> >>
> >> This new sysfs interface would help mkfs.btrfs to do more accurate
> >> warning.
> >>
> >> Signed-off-by: Qu Wenruo <w...@suse.com>
> >> ---
> >
> > Changes looks good. Nit below...
> > And maybe it is a good idea to wait for other comments before reroll.
> >
> >> fs/btrfs/sysfs.c | 34 ++++++++++++++++++++++++++++++++++
> >> 1 file changed, 34 insertions(+)
> >>
> >> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> >> index 6eb1c50fa98c..3ef419899472 100644
> >> --- a/fs/btrfs/sysfs.c
> >> +++ b/fs/btrfs/sysfs.c
> >> @@ -360,11 +360,45 @@ static ssize_t
> >> supported_rescue_options_show(struct kobject *kobj,
> >> BTRFS_ATTR(static_feature, supported_rescue_options,
> >> supported_rescue_options_show);
> >> +static ssize_t supported_ro_sectorsize_show(struct kobject *kobj,
> >> + struct kobj_attribute *a,
> >> + char *buf)
> >> +{
> >> + ssize_t ret = 0;
> >> + int i = 0;
> >
> > Drop variable i, as ret can be used instead of 'i'.
> >
> >> +
> >> + /* For 64K page size, 4K sector size is supported */
> >> + if (PAGE_SIZE == SZ_64K) {
> >> + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%u", SZ_4K);
> >> + i++;
> >> + }
> >> + /* Other than above subpage, only support PAGE_SIZE as sectorsize
> >> yet */
> >> + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%lu\n",
> >
> >> + (i ? " " : ""), PAGE_SIZE);
> > ^ret
> >
> >> + return ret;
> >> +}
> >> +BTRFS_ATTR(static_feature, supported_ro_sectorsize,
> >> + supported_ro_sectorsize_show);
> >> +
> >> +static ssize_t supported_rw_sectorsize_show(struct kobject *kobj,
> >> + struct kobj_attribute *a,
> >> + char *buf)
> >> +{
> >> + ssize_t ret = 0;
> >> +
> >> + /* Only PAGE_SIZE as sectorsize is supported */
> >> + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%lu\n", PAGE_SIZE);
> >> + return ret;
> >> +}
> >> +BTRFS_ATTR(static_feature, supported_rw_sectorsize,
> >> + supported_rw_sectorsize_show);
> >
> > Why not merge supported_ro_sectorsize and supported_rw_sectorsize
> > and show both in two lines...
> > For example:
> > cat supported_sectorsizes
> > ro: 4096 65536
> > rw: 65536
>
> If merged, btrfs-progs needs to do line number check before doing string
> matching.
The sysfs files should do one value per file.
> Although I doubt the usefulness for supported_ro_sectorsize, as the
> window for RO support without RW support should not be that large.
> (Current RW passes most generic test cases, and the remaining failures
> are very limited)
>
> Thus I can merged them into supported_sectorsize, and only report
> sectorsize we can do RW as supported.
In that case one file with the list of supported values is a better
option. The main point is to have full RW support, until then it's
interesting only for developers and they know what to expect.
