* UBI_READWRITE constraint when opening volumes to rename
@ 2014-10-07 14:31 Andrew Murray
2014-10-07 14:49 ` Ezequiel Garcia
2014-10-09 10:25 ` Ezequiel Garcia
0 siblings, 2 replies; 14+ messages in thread
From: Andrew Murray @ 2014-10-07 14:31 UTC (permalink / raw)
To: linux-mtd; +Cc: ezequiel.garcia, dedekind1
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.
If 'rename_volumes' is changed to UBI_READONLY then the above process
works.
I understand that this patch
https://patchwork.ozlabs.org/patch/339711/ recently changed the first
open restraint in 'rename_volumes' from UBI_EXCLUSIVE to
UBI_READWRITE. I'd like to understand if there are any risks in
changing it to UBI_READONLY?
Thanks,
Andrew Murray
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBI_READWRITE constraint when opening volumes to rename
2014-10-07 14:31 UBI_READWRITE constraint when opening volumes to rename Andrew Murray
@ 2014-10-07 14:49 ` Ezequiel Garcia
2014-10-09 10:25 ` Ezequiel Garcia
1 sibling, 0 replies; 14+ messages in thread
From: Ezequiel Garcia @ 2014-10-07 14:49 UTC (permalink / raw)
To: Andrew Murray; +Cc: Richard Weinberger, linux-mtd, dedekind1
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.
> If 'rename_volumes' is changed to UBI_READONLY then the above process
> works.
>
Maybe there's some way to tell UBIFS to pause any writes and switch
to read-only temporarily, while the rename takes place?
Does this make any sense?
> I understand that this patch
> https://patchwork.ozlabs.org/patch/339711/ recently changed the first
> open restraint in 'rename_volumes' from UBI_EXCLUSIVE to
> UBI_READWRITE. I'd like to understand if there are any risks in
> changing it to UBI_READONLY?
>
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBI_READWRITE constraint when opening volumes to rename
2014-10-07 14:31 UBI_READWRITE constraint when opening volumes to rename Andrew Murray
2014-10-07 14:49 ` Ezequiel Garcia
@ 2014-10-09 10:25 ` Ezequiel Garcia
2014-10-09 11:03 ` Andrew Murray
1 sibling, 1 reply; 14+ messages in thread
From: Ezequiel Garcia @ 2014-10-09 10:25 UTC (permalink / raw)
To: Andrew Murray; +Cc: Richard Weinberger, linux-mtd, dedekind1
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?
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!):
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;
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: UBI_READWRITE constraint when opening volumes to rename
2014-10-09 10:25 ` Ezequiel Garcia
@ 2014-10-09 11:03 ` Andrew Murray
2014-10-10 21:09 ` Richard Weinberger
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Murray @ 2014-10-09 11:03 UTC (permalink / raw)
To: Ezequiel Garcia; +Cc: Richard Weinberger, linux-mtd, Artem Bityutskiy
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.
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).
Thanks,
Andrew Murray
> --
> Ezequiel García, Free Electrons
> Embedded Linux, Kernel and Android Engineering
> http://free-electrons.com
--
Andrew Murray, Director
Embedded Bits Limited
www.embedded-bits.co.uk
Embedded Bits Limited is a company registered in England and Wales
with company number 08178608 and VAT number 140658911. Registered
office: Embedded Bits Limited c/o InTouch Accounting Ltd. Bristol and
West House Post Office Road Bournemouth Dorset BH1 1BL
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBI_READWRITE constraint when opening volumes to rename
2014-10-09 11:03 ` Andrew Murray
@ 2014-10-10 21:09 ` Richard Weinberger
2014-10-19 20:58 ` Andrew Murray
0 siblings, 1 reply; 14+ messages in thread
From: Richard Weinberger @ 2014-10-10 21:09 UTC (permalink / raw)
To: Andrew Murray, Ezequiel Garcia; +Cc: linux-mtd, Artem Bityutskiy
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
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
};
/**
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: UBI_READWRITE constraint when opening volumes to rename
2014-10-10 21:09 ` Richard Weinberger
@ 2014-10-19 20:58 ` Andrew Murray
2014-10-20 11:10 ` Richard Weinberger
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Murray @ 2014-10-19 20:58 UTC (permalink / raw)
To: Richard Weinberger; +Cc: linux-mtd, Ezequiel Garcia, Artem Bityutskiy
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBI_READWRITE constraint when opening volumes to rename
2014-10-19 20:58 ` Andrew Murray
@ 2014-10-20 11:10 ` Richard Weinberger
2014-10-20 11:40 ` Andrew Murray
2014-10-20 12:02 ` Artem Bityutskiy
0 siblings, 2 replies; 14+ messages in thread
From: Richard Weinberger @ 2014-10-20 11:10 UTC (permalink / raw)
To: Andrew Murray; +Cc: linux-mtd, Ezequiel Garcia, Artem Bityutskiy
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBI_READWRITE constraint when opening volumes to rename
2014-10-20 11:10 ` Richard Weinberger
@ 2014-10-20 11:40 ` Andrew Murray
2014-10-20 12:02 ` Artem Bityutskiy
1 sibling, 0 replies; 14+ messages in thread
From: Andrew Murray @ 2014-10-20 11:40 UTC (permalink / raw)
To: Richard Weinberger; +Cc: linux-mtd, Ezequiel Garcia, Artem Bityutskiy
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBI_READWRITE constraint when opening volumes to rename
2014-10-20 11:10 ` Richard Weinberger
2014-10-20 11:40 ` Andrew Murray
@ 2014-10-20 12:02 ` Artem Bityutskiy
2014-10-20 12:42 ` Richard Weinberger
1 sibling, 1 reply; 14+ messages in thread
From: Artem Bityutskiy @ 2014-10-20 12:02 UTC (permalink / raw)
To: Richard Weinberger; +Cc: Andrew Murray, linux-mtd, Ezequiel Garcia
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!
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBI_READWRITE constraint when opening volumes to rename
2014-10-20 12:02 ` Artem Bityutskiy
@ 2014-10-20 12:42 ` Richard Weinberger
2014-10-20 13:10 ` Artem Bityutskiy
2015-06-24 12:27 ` ir. Tjeerd Pinkert
0 siblings, 2 replies; 14+ messages in thread
From: Richard Weinberger @ 2014-10-20 12:42 UTC (permalink / raw)
To: dedekind1; +Cc: Andrew Murray, linux-mtd, Ezequiel Garcia
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBI_READWRITE constraint when opening volumes to rename
2014-10-20 12:42 ` Richard Weinberger
@ 2014-10-20 13:10 ` Artem Bityutskiy
2015-06-24 12:27 ` ir. Tjeerd Pinkert
1 sibling, 0 replies; 14+ messages in thread
From: Artem Bityutskiy @ 2014-10-20 13:10 UTC (permalink / raw)
To: Richard Weinberger; +Cc: Andrew Murray, linux-mtd, Ezequiel Garcia
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Re: UBI_READWRITE constraint when opening volumes to rename
2014-10-20 12:42 ` Richard Weinberger
2014-10-20 13:10 ` Artem Bityutskiy
@ 2015-06-24 12:27 ` ir. Tjeerd Pinkert
2015-06-25 7:52 ` Richard Weinberger
1 sibling, 1 reply; 14+ messages in thread
From: ir. Tjeerd Pinkert @ 2015-06-24 12:27 UTC (permalink / raw)
To: linux-mtd
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Re: UBI_READWRITE constraint when opening volumes to rename
2015-06-24 12:27 ` ir. Tjeerd Pinkert
@ 2015-06-25 7:52 ` Richard Weinberger
2015-06-25 9:28 ` ir. Tjeerd Pinkert
0 siblings, 1 reply; 14+ messages in thread
From: Richard Weinberger @ 2015-06-25 7:52 UTC (permalink / raw)
To: ir. Tjeerd Pinkert; +Cc: linux-mtd@lists.infradead.org
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,
//richard
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: UBI_READWRITE constraint when opening volumes to rename
2015-06-25 7:52 ` Richard Weinberger
@ 2015-06-25 9:28 ` ir. Tjeerd Pinkert
0 siblings, 0 replies; 14+ messages in thread
From: ir. Tjeerd Pinkert @ 2015-06-25 9:28 UTC (permalink / raw)
To: Richard Weinberger; +Cc: linux-mtd@lists.infradead.org
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
--
-----------------------------------------
Atomic, Molecular and Laser Physics Group
Office:
tel: ++31 20 5987858
room: T2.62
Lab:
tel: ++31 20 5987438
room: KA 173
tel: ++31 20 5987535
room: KA 151A
LaserLab, Vrije Universiteit
De Boelelaan 1081
1081 HV Amsterdam
The Netherlands
http://www.nat.vu.nl/en/research/atoms_molecules_lasers/
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-06-25 9:28 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-07 14:31 UBI_READWRITE constraint when opening volumes to rename Andrew Murray
2014-10-07 14:49 ` Ezequiel Garcia
2014-10-09 10:25 ` Ezequiel Garcia
2014-10-09 11:03 ` Andrew Murray
2014-10-10 21:09 ` Richard Weinberger
2014-10-19 20:58 ` Andrew Murray
2014-10-20 11:10 ` Richard Weinberger
2014-10-20 11:40 ` Andrew Murray
2014-10-20 12:02 ` Artem Bityutskiy
2014-10-20 12:42 ` Richard Weinberger
2014-10-20 13:10 ` Artem Bityutskiy
2015-06-24 12:27 ` ir. Tjeerd Pinkert
2015-06-25 7:52 ` Richard Weinberger
2015-06-25 9:28 ` ir. Tjeerd Pinkert
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).