On Tue, Mar 16, 2021 at 08:10:13AM +0800, Anand Jain wrote:
>
>
> On 16/03/2021 08:05, Qu Wenruo wrote:
> >
> >
> > On 2021/3/16 上午2:44, David Sterba wrote:
> >> 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.
> >>
> >
> > Indeed only full RW support makes sense.
> >
> Makes sense to me.
>
> > BTW, any comment on the file name? If no problem I would just use
> > "supported_sectorsize" in next update.
>
> supported_sectorsizes (plural) is better IMO.
Yeah pluar is consistent with what we have now, eg. supported_checksums
