diff mbox

UBI_READWRITE constraint when opening volumes to rename

Message ID 54384B11.6080209@nod.at
State RFC
Headers show

Commit Message

Richard Weinberger Oct. 10, 2014, 9:09 p.m. UTC 
Hi!

Am 09.10.2014 um 13:03 schrieb Andrew Murray:
> Hi Ezequiel,
> 
> On 9 October 2014 11:25, Ezequiel Garcia
> <ezequiel.garcia@free-electrons.com> wrote:
>> On 07 Oct 03:31 PM, Andrew Murray wrote:
>>> Hello,
>>>
>>> I'd like to be able to safely rename a UBI volume that contains a
>>> mounted UBIFS volume.
>>>
>>> This allows for firmware upgrade via the following steps:
>>>
>>> - ubimkvol rootfs_new
>>> - ubiupdatevol rootfs_new
>>> - ubirename rootfs rootfs_old rootfs_new rootfs
>>> - reboot
>>> - ubirmvol rootfs_old
>>>
>>> Such an approach makes upgrade of a root filesystem simple as there is
>>> no need to unmount the root filesystem. I believe this question has
>>> been asked before on this mailing list
>>> (http://lists.infradead.org/pipermail/linux-mtd/2012-February/039743.html).
>>>
>>> This process isn't possible at the moment as 'rename_volumes' opens
>>> the UBI volume with UBI_READWRITE. Unfortunately UBIFS always opens
>>> UBI with UBI_READWRITE regardless to if the user mounts as read-only.
>>
>> How about making UBIFS honour the read-only mount flag?
> 
> Thanks for the suggestion, I didn't consider this as I assumed there
> was a good reason for it being UBI_READWRITE - though it appears as
> though it's always been UBI_READWRITE since the initial
> implementation.
> 
>>
>> A quick look to fs/ubifs/io.c shows that UBIFS will show an error message
>> if a LEB erase/write operation is attempted on a read-only mounted or
>> read-only media. So, hopefully, this is reasonable (the patch is completely
>> untested!):
> 
> Is there any reason why UBIFS would need to write to UBI when the user
> mounts UBIFS as R/O? I know that scrubbing can occur in the UBI layer
> - will this still occur if the volume is opened as UBI_READONLY?
> 
>>
>> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
>> index 106bf20..ce445ce 100644
>> --- a/fs/ubifs/super.c
>> +++ b/fs/ubifs/super.c
>> @@ -1998,11 +1998,16 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
>>  {
>>         struct ubifs_info *c = sb->s_fs_info;
>>         struct inode *root;
>> -       int err;
>> +       int err, mode;
>> +
>> +       /*
>> +        * Re-open the UBI device in read-write mode, or keep it read-only if
>> +        * explicitly requested.
>> +        */
>> +       mode = (sb->s_flags & MS_RDONLY) ? UBI_READONLY : UBI_READWRITE;
>>
>>         c->vfs_sb = sb;
>> -       /* Re-open the UBI device in read-write mode */
>> -       c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, UBI_READWRITE);
>> +       c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, mode);
>>         if (IS_ERR(c->ubi)) {
>>                 err = PTR_ERR(c->ubi);
>>                 goto out;
>>
> 
> I will try this later and get back to you - this seems like the right fix.

As discussed with Ezequiel on IRC the right thing is to fix the UBI rename logic.

> To satisfy my curiosity - does the UBI rename function really need to
> open the UBI volume as UBI_READWRITE to rename? Does it matter that a
> UBIFS fileystem may be writing UBI whilst a UBI rename occurs - I've
> assumed that UBIFS doesn't ever read/write the volume label - so it
> doesn't matter if it changes beneath it? (I'm not too familiar with
> UBI/UBIFS internals).

Correct. Renaming a volume does not alter nor read volume data.
Only the UBI volume table is altered.
This is why I've cooked up the following patch.
Please give it some testing!

Thanks,
//richard

Comments

Andrew Murray Oct. 19, 2014, 8:58 p.m. UTC | #1 
On 10 October 2014 22:09, Richard Weinberger <richard@nod.at> wrote:
>
> As discussed with Ezequiel on IRC the right thing is to fix the UBI rename logic.
>
>> To satisfy my curiosity - does the UBI rename function really need to
>> open the UBI volume as UBI_READWRITE to rename? Does it matter that a
>> UBIFS fileystem may be writing UBI whilst a UBI rename occurs - I've
>> assumed that UBIFS doesn't ever read/write the volume label - so it
>> doesn't matter if it changes beneath it? (I'm not too familiar with
>> UBI/UBIFS internals).
>
> Correct. Renaming a volume does not alter nor read volume data.
> Only the UBI volume table is altered.
> This is why I've cooked up the following patch.
> Please give it some testing!

This appears to work for me - I can rename a mounted volume without
any obvious side effects. I've slightly tweaked (added a break
statement) and submitted your patch.

Though I've had a further look and have an additional questions.

This patch means that UBI_EXCLUSIVE is no longer exclusive (unless
these constraints are with respect to the volume data only and not the
volume table as well) - thus should we update the ubi_open_volume
function to prevent UBI_EXCLUSIVE when metaonly is already held (and
to prevent UBI_METAONLY when exclusive is already held)? I can't see
the harm in doing this and it probably won't impact the use case that
led to this patch.

I can't see any locking around the table fields and thus (with present
patch) I assume you could perform a rename and volume remove at the
same time - I wonder if this would sometimes break. I assume that
anything that currently modifies the volume table will be doing so
with exclusive held.

Thanks,

Andrew Murray
Richard Weinberger Oct. 20, 2014, 11:10 a.m. UTC | #2 
Am 19.10.2014 um 22:58 schrieb Andrew Murray:
> On 10 October 2014 22:09, Richard Weinberger <richard@nod.at> wrote:
>>
>> As discussed with Ezequiel on IRC the right thing is to fix the UBI rename logic.
>>
>>> To satisfy my curiosity - does the UBI rename function really need to
>>> open the UBI volume as UBI_READWRITE to rename? Does it matter that a
>>> UBIFS fileystem may be writing UBI whilst a UBI rename occurs - I've
>>> assumed that UBIFS doesn't ever read/write the volume label - so it
>>> doesn't matter if it changes beneath it? (I'm not too familiar with
>>> UBI/UBIFS internals).
>>
>> Correct. Renaming a volume does not alter nor read volume data.
>> Only the UBI volume table is altered.
>> This is why I've cooked up the following patch.
>> Please give it some testing!
> 
> This appears to work for me - I can rename a mounted volume without
> any obvious side effects. I've slightly tweaked (added a break
> statement) and submitted your patch.
> 
> Though I've had a further look and have an additional questions.
> 
> This patch means that UBI_EXCLUSIVE is no longer exclusive (unless
> these constraints are with respect to the volume data only and not the
> volume table as well) - thus should we update the ubi_open_volume
> function to prevent UBI_EXCLUSIVE when metaonly is already held (and
> to prevent UBI_METAONLY when exclusive is already held)? I can't see
> the harm in doing this and it probably won't impact the use case that
> led to this patch.
>

Correct. See my first mail. :)

> I can't see any locking around the table fields and thus (with present
> patch) I assume you could perform a rename and volume remove at the
> same time - I wonder if this would sometimes break. I assume that
> anything that currently modifies the volume table will be doing so
> with exclusive held.

This is why I need to review all code paths first.
My initial patch was not supposed to be a final solution, more a base for discussion.
I.e. to follow the "less talk, more code" rule.

Thanks,
//richard
Andrew Murray Oct. 20, 2014, 11:40 a.m. UTC | #3 
On 20 October 2014 12:10, Richard Weinberger <richard@nod.at> wrote:
> Am 19.10.2014 um 22:58 schrieb Andrew Murray:
>> On 10 October 2014 22:09, Richard Weinberger <richard@nod.at> wrote:
>>>
>>> As discussed with Ezequiel on IRC the right thing is to fix the UBI rename logic.
>>>
>>>> To satisfy my curiosity - does the UBI rename function really need to
>>>> open the UBI volume as UBI_READWRITE to rename? Does it matter that a
>>>> UBIFS fileystem may be writing UBI whilst a UBI rename occurs - I've
>>>> assumed that UBIFS doesn't ever read/write the volume label - so it
>>>> doesn't matter if it changes beneath it? (I'm not too familiar with
>>>> UBI/UBIFS internals).
>>>
>>> Correct. Renaming a volume does not alter nor read volume data.
>>> Only the UBI volume table is altered.
>>> This is why I've cooked up the following patch.
>>> Please give it some testing!
>>
>> This appears to work for me - I can rename a mounted volume without
>> any obvious side effects. I've slightly tweaked (added a break
>> statement) and submitted your patch.
>>
>> Though I've had a further look and have an additional questions.
>>
>> This patch means that UBI_EXCLUSIVE is no longer exclusive (unless
>> these constraints are with respect to the volume data only and not the
>> volume table as well) - thus should we update the ubi_open_volume
>> function to prevent UBI_EXCLUSIVE when metaonly is already held (and
>> to prevent UBI_METAONLY when exclusive is already held)? I can't see
>> the harm in doing this and it probably won't impact the use case that
>> led to this patch.
>>
>
> Correct. See my first mail. :)
>
>> I can't see any locking around the table fields and thus (with present
>> patch) I assume you could perform a rename and volume remove at the
>> same time - I wonder if this would sometimes break. I assume that
>> anything that currently modifies the volume table will be doing so
>> with exclusive held.
>
> This is why I need to review all code paths first.
> My initial patch was not supposed to be a final solution, more a base for discussion.
> I.e. to follow the "less talk, more code" rule.

Sure I understand.

I'm happy to further investigate this and propose a v2.

Andrew Murray

>
> Thanks,
> //richard
Artem Bityutskiy Oct. 20, 2014, 12:02 p.m. UTC | #4 
On Mon, 2014-10-20 at 13:10 +0200, Richard Weinberger wrote:
> This is why I need to review all code paths first.
> My initial patch was not supposed to be a final solution, more a base for discussion.
> I.e. to follow the "less talk, more code" rule.

Let me try to summarise.

Exclusive mode - used for volume and LEB update. We do not want someone
to race with these operations on the same LEBs. Indeed, if one performs
a volume or LEB update, we want to guarantee that that the result of the
operation is that the volume/LEB contains the data user sent us.

Read/write - just R/W mode, many users may race

Read-only - when we know we should not write to the volume, and want UBI
to refuse our writes in case we try to write, say, because of a bug in
UBIFS code.

AFAICS, all the modes are useful.

Metaonly - we are not going to change the data, only the meta-data like
the volume name. Seem to be a good idea to me, thanks!
Richard Weinberger Oct. 20, 2014, 12:42 p.m. UTC | #5 
Am 20.10.2014 um 14:02 schrieb Artem Bityutskiy:
> On Mon, 2014-10-20 at 13:10 +0200, Richard Weinberger wrote:
>> This is why I need to review all code paths first.
>> My initial patch was not supposed to be a final solution, more a base for discussion.
>> I.e. to follow the "less talk, more code" rule.
> 
> Let me try to summarise.
> 
> Exclusive mode - used for volume and LEB update. We do not want someone
> to race with these operations on the same LEBs. Indeed, if one performs
> a volume or LEB update, we want to guarantee that that the result of the
> operation is that the volume/LEB contains the data user sent us.
> 
> Read/write - just R/W mode, many users may race
> 
> Read-only - when we know we should not write to the volume, and want UBI
> to refuse our writes in case we try to write, say, because of a bug in
> UBIFS code.
> 
> AFAICS, all the modes are useful.
> 
> Metaonly - we are not going to change the data, only the meta-data like
> the volume name. Seem to be a good idea to me, thanks!

BTW: one corner case is the volume delete operation.
It touches meta data and LEBs. Currently you need exclusive mode for this.
IMHO it makes sense to deny metaonly mode if an UBI volume is opened in exclusive mode.

Thanks,
//richard
Artem Bityutskiy Oct. 20, 2014, 1:10 p.m. UTC | #6 
On Mon, 2014-10-20 at 14:42 +0200, Richard Weinberger wrote:
> BTW: one corner case is the volume delete operation.
> It touches meta data and LEBs. Currently you need exclusive mode for this.
> IMHO it makes sense to deny metaonly mode if an UBI volume is opened in exclusive mode.

Oh, yes, the 'delete' semantics is that volume goes away and all the
space it had reserved becomes available, so the data it contained is
thrown away, so the operation does affect the data.

Artem.
ir. Tjeerd Pinkert June 24, 2015, 12:27 p.m. UTC | #7 
I'm just wondering, has this been included in the distro/kernel already?
Or is it only in the git repo?

I could not perform the rename action with ro mounted / with v1.5.1
programs and 3.18.6 stock kernel (buildroot system).

Best regards,


Tjeerd
Richard Weinberger June 25, 2015, 7:52 a.m. UTC | #8 
On Wed, Jun 24, 2015 at 2:27 PM, ir. Tjeerd Pinkert <t.j.pinkert@vu.nl> wrote:
> I'm just wondering, has this been included in the distro/kernel already?
> Or is it only in the git repo?

"the distro"? The feature is mainline since 4.0.
ir. Tjeerd Pinkert June 25, 2015, 9:28 a.m. UTC | #9 
On 25-06-15 09:52, Richard Weinberger wrote:
> On Wed, Jun 24, 2015 at 2:27 PM, ir. Tjeerd Pinkert <t.j.pinkert@vu.nl> wrote:
>> I'm just wondering, has this been included in the distro/kernel already?
>> Or is it only in the git repo?
> 
> "the distro"? The feature is mainline since 4.0.

Thanks, I will update my kernel. (With distro I meant the distribution
package (v1.5.1) of the tools)

Yours,


Tjeerd
diff mbox

Patch

diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index 7646220..4c4c455 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -731,7 +731,7 @@  static int rename_volumes(struct ubi_device *ubi,
 			goto out_free;
 		}

-		re->desc = ubi_open_volume(ubi->ubi_num, vol_id, UBI_READWRITE);
+		re->desc = ubi_open_volume(ubi->ubi_num, vol_id, UBI_METAONLY);
 		if (IS_ERR(re->desc)) {
 			err = PTR_ERR(re->desc);
 			ubi_err("cannot open volume %d, error %d", vol_id, err);
diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c
index 3aac1ac..cfc49cd 100644
--- a/drivers/mtd/ubi/kapi.c
+++ b/drivers/mtd/ubi/kapi.c
@@ -137,7 +137,7 @@  struct ubi_volume_desc *ubi_open_volume(int ubi_num, int vol_id, int mode)
 		return ERR_PTR(-EINVAL);

 	if (mode != UBI_READONLY && mode != UBI_READWRITE &&
-	    mode != UBI_EXCLUSIVE)
+	    mode != UBI_EXCLUSIVE && mode != UBI_METAONLY)
 		return ERR_PTR(-EINVAL);

 	/*
@@ -186,6 +186,10 @@  struct ubi_volume_desc *ubi_open_volume(int ubi_num, int vol_id, int mode)
 			goto out_unlock;
 		vol->exclusive = 1;
 		break;
+	case UBI_METAONLY:
+		if (vol->metaonly)
+			goto out_unlock;
+		vol->metaonly = 1;
 	}
 	get_device(&vol->dev);
 	vol->ref_count += 1;
@@ -343,6 +347,8 @@  void ubi_close_volume(struct ubi_volume_desc *desc)
 		break;
 	case UBI_EXCLUSIVE:
 		vol->exclusive = 0;
+	case UBI_METAONLY:
+		vol->metaonly = 0;
 	}
 	vol->ref_count -= 1;
 	spin_unlock(&ubi->volumes_lock);
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 7bf4163..629973f 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -260,6 +260,7 @@  struct ubi_fm_pool {
  * @readers: number of users holding this volume in read-only mode
  * @writers: number of users holding this volume in read-write mode
  * @exclusive: whether somebody holds this volume in exclusive mode
+ * @metaonly: whether somebody is altering only meta data of this volume
  *
  * @reserved_pebs: how many physical eraseblocks are reserved for this volume
  * @vol_type: volume type (%UBI_DYNAMIC_VOLUME or %UBI_STATIC_VOLUME)
@@ -308,6 +309,7 @@  struct ubi_volume {
 	int readers;
 	int writers;
 	int exclusive;
+	int metaonly;

 	int reserved_pebs;
 	int vol_type;
@@ -389,7 +391,8 @@  struct ubi_debug_info {
  * @volumes_lock: protects @volumes, @rsvd_pebs, @avail_pebs, beb_rsvd_pebs,
  *                @beb_rsvd_level, @bad_peb_count, @good_peb_count, @vol_count,
  *                @vol->readers, @vol->writers, @vol->exclusive,
- *                @vol->ref_count, @vol->mapping and @vol->eba_tbl.
+ *                @vol->metaonly, @vol->ref_count, @vol->mapping and
+ *                @vol->eba_tbl.
  * @ref_count: count of references on the UBI device
  * @image_seq: image sequence number recorded on EC headers
  *
diff --git a/include/linux/mtd/ubi.h b/include/linux/mtd/ubi.h
index c3918a0..262aae1 100644
--- a/include/linux/mtd/ubi.h
+++ b/include/linux/mtd/ubi.h
@@ -34,11 +34,13 @@ 
  * UBI_READONLY: read-only mode
  * UBI_READWRITE: read-write mode
  * UBI_EXCLUSIVE: exclusive mode
+ * UBI_METAONLY: modify voume meta data only
  */
 enum {
 	UBI_READONLY = 1,
 	UBI_READWRITE,
-	UBI_EXCLUSIVE
+	UBI_EXCLUSIVE,
+	UBI_METAONLY
 };

 /**