Adding subpage support to NAND driver -- backwards compatibility concerns

Adding subpage support to NAND driver -- backwards compatibility concerns

[Ben Shelton]

Hi all,

We're currently carrying a patch out of tree to add subpage read and
write support to the pl353_nand driver.  Xilinx is currently working to
mainline this driver; see

http://www.spinics.net/lists/devicetree/msg76307.html

We'd like to upstream our patch, but my concern is that UBIFS behaves
differently when it knows that the flash device supports subpages.  I
have a couple of questions related to that:

- I know from experience that bad things happen when you use a kernel
  without subpage support with an UBIFS filesystem that was formatted
  with subpage support.  Is it safe to do the opposite (kernel with
  subpage support / UBIFS filesystem formatted without subpage support)?

- Assuming that it isn't safe, what's the best way to add subpage
  support to this driver in an upstreamable way / without breaking
  people?  Would it be sufficient to add subpage support as a Kconfig
  option that's disabled by default with a strongly-worded message
  describing the consequences of enabling it?

Thanks,
Ben

RE: Adding subpage support to NAND driver -- backwards compatibility concerns

[Iwo Mergler]

On Thu, 23 Apr 2015 03:29:44 +1000
Ben Shelton <ben.shelton@ni.com> wrote:

> We'd like to upstream our patch, but my concern is that UBIFS behaves
> differently when it knows that the flash device supports subpages.  I
> have a couple of questions related to that:
> 
> - I know from experience that bad things happen when you use a kernel
>   without subpage support with an UBIFS filesystem that was formatted
>   with subpage support.  Is it safe to do the opposite (kernel with
>   subpage support / UBIFS filesystem formatted without subpage
> support)?
> 
> - Assuming that it isn't safe, what's the best way to add subpage
>   support to this driver in an upstreamable way / without breaking
>   people?  Would it be sufficient to add subpage support as a Kconfig
>   option that's disabled by default with a strongly-worded message
>   describing the consequences of enabling it?

Hi Ben,


from what I understand, the only part of the UBI/UBIFS stack that
uses / cares about subpages are the UBI EC and VID headers. If you
have subpage access, the two headers will share a page, if not, they
live in separate pages. With subpages, you half your UBI overhead.

This affects the LEB size for UBIFS as well as the UBI header and data
locations within the PEB, so the filesystems are incompatible.

If you add subpage support to a system that previously had none, and
presumably want to use the old file systems, you need to force the
ubiattach command to use the page size as the VID header offset.

Something like

	PAGESIZE=`cat /sys/class/mtd/mtd0/writesize`
	ubiattach /dev/ubi_ctrl -O $PAGESIZE ...

Same applies to any ubiformat commands.

This stops UBI from using the subpage capability. You also don't
get the benefit of the lower overhead, of course.

Traditionally, if someone changes the kernel config, breaking things
is definitely expected consequences. So, making subpage support
a default-off option for the driver has my vote.


Best regards,

Iwo

Re: Adding subpage support to NAND driver -- backwards compatibility concerns

[Josh Cartwright]

[-- Attachment #1: Type: text/plain, Size: 3062 bytes --]

+Richard, who, when not being trolled on IRC, has been working on
UBI(FS) stuff.

On Thu, Apr 23, 2015 at 12:48:53PM +1000, Iwo Mergler wrote:
> On Thu, 23 Apr 2015 03:29:44 +1000
> Ben Shelton <ben.shelton@ni.com> wrote:
> > We'd like to upstream our patch, but my concern is that UBIFS behaves
> > differently when it knows that the flash device supports subpages.  I
> > have a couple of questions related to that:
> > 
> > - I know from experience that bad things happen when you use a kernel
> >   without subpage support with an UBIFS filesystem that was formatted
> >   with subpage support.  Is it safe to do the opposite (kernel with
> >   subpage support / UBIFS filesystem formatted without subpage
> > support)?
> > 
> > - Assuming that it isn't safe, what's the best way to add subpage
> >   support to this driver in an upstreamable way / without breaking
> >   people?  Would it be sufficient to add subpage support as a Kconfig
> >   option that's disabled by default with a strongly-worded message
> >   describing the consequences of enabling it?
[..]
> from what I understand, the only part of the UBI/UBIFS stack that
> uses / cares about subpages are the UBI EC and VID headers.

Are the locations of both headers changed when subpage accesses are
supported?  I was under the impression that EC was always at the
beginning of the page, with the VID headers at the next min IO boundary.
(So, only the location of the VID header would be changed).

> If you have subpage access, the two headers will share a page, if not,
> they live in separate pages. With subpages, you half your UBI
> overhead.
> 
> This affects the LEB size for UBIFS as well as the UBI header and data
> locations within the PEB, so the filesystems are incompatible.
> 
> If you add subpage support to a system that previously had none, and
> presumably want to use the old file systems, you need to force the
> ubiattach command to use the page size as the VID header offset.

Okay, well; I would expect that for some systems that are using UBIFS as
root, tweaking the commandline to add 'ubi.mtd=0,<size>' would require a
bootloader change.

Anyway, I think we're talking only about theoretical breakage here, so
it's reasonable to ask whether or not we should even care about this at
all.

> Something like
> 
> 	PAGESIZE=`cat /sys/class/mtd/mtd0/writesize`
> 	ubiattach /dev/ubi_ctrl -O $PAGESIZE ...
> 
> Same applies to any ubiformat commands.
> 
> This stops UBI from using the subpage capability. You also don't
> get the benefit of the lower overhead, of course.
> 
> Traditionally, if someone changes the kernel config, breaking things
> is definitely expected consequences. So, making subpage support
> a default-off option for the driver has my vote.

Is there no metadata in the UBI data structures in flash that indicate
the min IO boundary?  Assuming no, is another option to, at the time of
attach, try both the min IO access size, and, if that doesn't work, try
the page size?

  Josh

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: Adding subpage support to NAND driver -- backwards compatibility concerns

[Richard Weinberger]

Am 23.04.2015 um 20:39 schrieb Josh Cartwright:
> +Richard, who, when not being trolled on IRC, has been working on
> UBI(FS) stuff.

Yeah, being on #kernelnewbeis is always "fun". ;-)

> On Thu, Apr 23, 2015 at 12:48:53PM +1000, Iwo Mergler wrote:
>> On Thu, 23 Apr 2015 03:29:44 +1000
>> Ben Shelton <ben.shelton@ni.com> wrote:
>>> We'd like to upstream our patch, but my concern is that UBIFS behaves
>>> differently when it knows that the flash device supports subpages.  I
>>> have a couple of questions related to that:
>>>
>>> - I know from experience that bad things happen when you use a kernel
>>>   without subpage support with an UBIFS filesystem that was formatted
>>>   with subpage support.  Is it safe to do the opposite (kernel with
>>>   subpage support / UBIFS filesystem formatted without subpage
>>> support)?
>>>
>>> - Assuming that it isn't safe, what's the best way to add subpage
>>>   support to this driver in an upstreamable way / without breaking
>>>   people?  Would it be sufficient to add subpage support as a Kconfig
>>>   option that's disabled by default with a strongly-worded message
>>>   describing the consequences of enabling it?
> [..]
>> from what I understand, the only part of the UBI/UBIFS stack that
>> uses / cares about subpages are the UBI EC and VID headers.
> 
> Are the locations of both headers changed when subpage accesses are
> supported?  I was under the impression that EC was always at the
> beginning of the page, with the VID headers at the next min IO boundary.
> (So, only the location of the VID header would be changed).

This is correct. Only the VID header should be changed.
Using ubiattach'S --vid-hdr-offset you can tell UBI about
subpages.

>> If you have subpage access, the two headers will share a page, if not,
>> they live in separate pages. With subpages, you half your UBI
>> overhead.
>>
>> This affects the LEB size for UBIFS as well as the UBI header and data
>> locations within the PEB, so the filesystems are incompatible.
>>
>> If you add subpage support to a system that previously had none, and
>> presumably want to use the old file systems, you need to force the
>> ubiattach command to use the page size as the VID header offset.
> 
> Okay, well; I would expect that for some systems that are using UBIFS as
> root, tweaking the commandline to add 'ubi.mtd=0,<size>' would require a
> bootloader change.
> 
> Anyway, I think we're talking only about theoretical breakage here, so
> it's reasonable to ask whether or not we should even care about this at
> all.
> 
>> Something like
>>
>> 	PAGESIZE=`cat /sys/class/mtd/mtd0/writesize`
>> 	ubiattach /dev/ubi_ctrl -O $PAGESIZE ...
>>
>> Same applies to any ubiformat commands.
>>
>> This stops UBI from using the subpage capability. You also don't
>> get the benefit of the lower overhead, of course.
>>
>> Traditionally, if someone changes the kernel config, breaking things
>> is definitely expected consequences. So, making subpage support
>> a default-off option for the driver has my vote.
> 
> Is there no metadata in the UBI data structures in flash that indicate
> the min IO boundary?  Assuming no, is another option to, at the time of
> attach, try both the min IO access size, and, if that doesn't work, try
> the page size?

Correct. UBI has no information about that.
If you add subpage support to the driver I'd make it opt-in such that
existing setups won't break.

Thanks,
//richard

RE: Adding subpage support to NAND driver -- backwards compatibility concerns

[Iwo Mergler]

On Fri, 24 Apr 2015 05:30:55 +1000
Richard Weinberger <richard@nod.at> wrote:

> Am 23.04.2015 um 20:39 schrieb Josh Cartwright:

> > Is there no metadata in the UBI data structures in flash that
> > indicate the min IO boundary?  Assuming no, is another option to,
> > at the time of attach, try both the min IO access size, and, if
> > that doesn't work, try the page size?
> 
> Correct. UBI has no information about that.
> If you add subpage support to the driver I'd make it opt-in such that
> existing setups won't break.

I'm wondering, given that EC headers contain vid header offset
and data offset fields, shouldn't UBI be able to deduce at attach
time what the relevant parameters are on a partition?

Something along the lines of using the parameters of the first
PEB with valid EC header, then balking if another EC header is
encountered with different info. As long as MTD allows the
so deduced minimum access size, it seems safe.

This could help enormously in situations like this, where
MTD drivers experience sudden bursts of improvement.


Best regards,

Iwo

Re: Adding subpage support to NAND driver -- backwards compatibility concerns

[Richard Weinberger]

Am 24.04.2015 um 01:13 schrieb Iwo Mergler:
> On Fri, 24 Apr 2015 05:30:55 +1000
> Richard Weinberger <richard@nod.at> wrote:
> 
>> Am 23.04.2015 um 20:39 schrieb Josh Cartwright:
> 
>>> Is there no metadata in the UBI data structures in flash that
>>> indicate the min IO boundary?  Assuming no, is another option to,
>>> at the time of attach, try both the min IO access size, and, if
>>> that doesn't work, try the page size?
>>
>> Correct. UBI has no information about that.
>> If you add subpage support to the driver I'd make it opt-in such that
>> existing setups won't break.
> 
> I'm wondering, given that EC headers contain vid header offset
> and data offset fields, shouldn't UBI be able to deduce at attach
> time what the relevant parameters are on a partition?

UBI double checks that offset. If the vid header offset does not
match the run-time computed offset UBI will detect that in
validate_ec_hdr().

So, yes it could automatically deduce it at attach time but it does not
for safety reasons. UBI computes the values from the data provided by
the MTD, if the image does not match it stops.
If it would blindly trust the image it could happen that you operate on your
MTD with a wrong page size.

> Something along the lines of using the parameters of the first
> PEB with valid EC header, then balking if another EC header is
> encountered with different info. As long as MTD allows the
> so deduced minimum access size, it seems safe.
> 
> This could help enormously in situations like this, where
> MTD drivers experience sudden bursts of improvement.

Agreed. UBI could be more intelligent. If UBI can be sure that
the parameters deduced from the on-flash image are safe with
respect to the parameters provided by MTD it could continue.

Maybe Artem can give us more details on that as he designed it.

Thanks,
//richard