* Re: [PATCH] bcache: don't attach backing with duplicate UUID
@ 2018-03-05 1:07 tang.junhui
2018-03-05 21:46 ` Michael Lyle
0 siblings, 1 reply; 6+ messages in thread
From: tang.junhui @ 2018-03-05 1:07 UTC (permalink / raw)
To: mlyle; +Cc: linux-block, linux-bcache, tang.junhui
Hello Mike
I send the email from my personal mailbox(110950397@qq.com), it may be fail,
so I resend this email from my office mailbox again. bellow is the mail
context I send previous.
====================================================
I am Tang Junhui(tang.junhui@zte.com.cn), This email comes from my
personal mailbox, since I am not in office today.
> > From: Tang Junhui <tang.junhui@xxxxxxxxxx>
> >
> > Hello, Mike
> >
> > This patch looks good, but has some conflicts with this patch:
> > bcache: fix for data collapse after re-attaching an attached device
> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=73ac105be390c1de42a2f21643c9778a5e002930
> > Could you modify your fix base on the previous patch?
> That doesn't make sense. This patch was generated from a current tree
> where it's applied on top of that: (It's based on next when it should
> really be based on Linus's tree, but it doesn't matter for patch
> application because there's no changes in next right now to bcache that
> aren't in Linus's tree).
Originally, I did not mean merger conflicts, but the
code logical conflicts, since the previous patch add a new input parameter
set_uuid in bch_cached_dev_attach(), and if set_uuid is not NULL,
we use set_uuid as cache set uuid, otherwise, we use dc->sb.set_uuid as
the cache set uuid.
But now, I read your patch again, and realize that you did not use
dc->sb.set_uuid, but use dc->sb.uuid to judge whether the device is a
duplicate backend device, so it's OK for me.
> May I add your reviewed-by so I can send this (and your fix) upstream?
Reviewed-by: Tang Junhui <tang.junhui@zte.com.cn>
Thanks
Tang Junhui
Thanks
Tang Junhui
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bcache: don't attach backing with duplicate UUID
2018-03-05 1:07 [PATCH] bcache: don't attach backing with duplicate UUID tang.junhui
@ 2018-03-05 21:46 ` Michael Lyle
0 siblings, 0 replies; 6+ messages in thread
From: Michael Lyle @ 2018-03-05 21:46 UTC (permalink / raw)
To: tang.junhui; +Cc: linux-block, linux-bcache
Hi Tang Junhui---
Thanks for your review. I just sent it upstream (with your change) to Jens.
Mike
On 03/04/2018 05:07 PM, tang.junhui@zte.com.cn wrote:
> Hello Mike
>
> I send the email from my personal mailbox(110950397@qq.com), it may be fail,
> so I resend this email from my office mailbox again. bellow is the mail
> context I send previous.
>
> ====================================================
>
> I am Tang Junhui(tang.junhui@zte.com.cn), This email comes from my
> personal mailbox, since I am not in office today.
>
>>> From: Tang Junhui <tang.junhui@xxxxxxxxxx>
>>>
>>> Hello, Mike
>>>
>>> This patch looks good, but has some conflicts with this patch:
>>> bcache: fix for data collapse after re-attaching an attached device
>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=73ac105be390c1de42a2f21643c9778a5e002930
>>> Could you modify your fix base on the previous patch?
>
>> That doesn't make sense. This patch was generated from a current tree
>> where it's applied on top of that: (It's based on next when it should
>> really be based on Linus's tree, but it doesn't matter for patch
>> application because there's no changes in next right now to bcache that
>> aren't in Linus's tree).
>
> Originally, I did not mean merger conflicts, but the
> code logical conflicts, since the previous patch add a new input parameter
> set_uuid in bch_cached_dev_attach(), and if set_uuid is not NULL,
> we use set_uuid as cache set uuid, otherwise, we use dc->sb.set_uuid as
> the cache set uuid.
>
> But now, I read your patch again, and realize that you did not use
> dc->sb.set_uuid, but use dc->sb.uuid to judge whether the device is a
> duplicate backend device, so it's OK for me.
>
>> May I add your reviewed-by so I can send this (and your fix) upstream?
> Reviewed-by: Tang Junhui <tang.junhui@zte.com.cn>
>
> Thanks
> Tang Junhui
>
> Thanks
> Tang Junhui
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bcache: don't attach backing with duplicate UUID
2018-03-02 14:55 ` Michael Lyle
@ 2018-03-02 14:59 ` Michael Lyle
0 siblings, 0 replies; 6+ messages in thread
From: Michael Lyle @ 2018-03-02 14:59 UTC (permalink / raw)
To: tang.junhui; +Cc: linux-bcache, linux-block
On 03/02/2018 06:55 AM, Michael Lyle wrote:
> Hi Tang Junhui---
>
> On 03/01/2018 09:45 PM, tang.junhui@zte.com.cn wrote:
>> From: Tang Junhui <tang.junhui@zte.com.cn>
>>
>> Hello, Mike
>>
>> This patch looks good, but has some conflicts with this patch:
>> bcache: fix for data collapse after re-attaching an attached device
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=73ac105be390c1de42a2f21643c9778a5e002930
>> Could you modify your fix base on the previous patch?
>
> That doesn't make sense. This patch was generated from a current tree
> where it's applied on top of that: (It's based on next when it should
> really be based on Linus's tree, but it doesn't matter for patch
> application because there's no changes in next right now to bcache that
> aren't in Linus's tree).
>
> https://github.com/mlyle/linux/commit/502c38831b42100ac5d3319c792a66c7e01357ae
>
> May I add your reviewed-by so I can send this (and your fix) upstream?
>
> Mike
Just to confirm this all works---
mlyle@midnight:~/trees/linux$ git checkout linus/master
Note: checking out 'linus/master'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.
If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:
git checkout -b <new-branch-name>
HEAD is now at 5d60e057d127... Merge tag 'drm-fixes-for-v4.16-rc4' of
git://people.freedesktop.org/~airlied/linux
mlyle@midnight:~/trees/linux$ git am
0001-bcache-fix-crashes-in-duplicate-cache-device-registe.patch
Applying: bcache: fix crashes in duplicate cache device register
mlyle@midnight:~/trees/linux$ git am
0001-bcache-don-t-attach-backing-with-duplicate-UUID.patch
Applying: bcache: don't attach backing with duplicate UUID
Mike
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bcache: don't attach backing with duplicate UUID
2018-03-02 5:45 tang.junhui
@ 2018-03-02 14:55 ` Michael Lyle
2018-03-02 14:59 ` Michael Lyle
0 siblings, 1 reply; 6+ messages in thread
From: Michael Lyle @ 2018-03-02 14:55 UTC (permalink / raw)
To: tang.junhui; +Cc: linux-bcache, linux-block
Hi Tang Junhui---
On 03/01/2018 09:45 PM, tang.junhui@zte.com.cn wrote:
> From: Tang Junhui <tang.junhui@zte.com.cn>
>
> Hello, Mike
>
> This patch looks good, but has some conflicts with this patch:
> bcache: fix for data collapse after re-attaching an attached device
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=73ac105be390c1de42a2f21643c9778a5e002930
> Could you modify your fix base on the previous patch?
That doesn't make sense. This patch was generated from a current tree
where it's applied on top of that: (It's based on next when it should
really be based on Linus's tree, but it doesn't matter for patch
application because there's no changes in next right now to bcache that
aren't in Linus's tree).
https://github.com/mlyle/linux/commit/502c38831b42100ac5d3319c792a66c7e01357ae
May I add your reviewed-by so I can send this (and your fix) upstream?
Mike
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bcache: don't attach backing with duplicate UUID
@ 2018-03-02 5:45 tang.junhui
2018-03-02 14:55 ` Michael Lyle
0 siblings, 1 reply; 6+ messages in thread
From: tang.junhui @ 2018-03-02 5:45 UTC (permalink / raw)
To: mlyle; +Cc: linux-bcache, linux-block, tang.junhui
From: Tang Junhui <tang.junhui@zte.com.cn>
Hello, Mike
This patch looks good, but has some conflicts with this patch:
bcache: fix for data collapse after re-attaching an attached device
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=73ac105be390c1de42a2f21643c9778a5e002930
Could you modify your fix base on the previous patch?
> This can happen e.g. during disk cloning.
>
> This is an incomplete fix: it does not catch duplicate UUIDs earlier
> when things are still unattached. It does not unregister the device.
> Further changes to cope better with this are planned but conflict with
> Coly's ongoing improvements to handling device errors. In the meantime,
> one can manually stop the device after this has happened.
>
> Attempts to attach a duplicate device result in:
>
> [ 136.372404] loop: module loaded
> [ 136.424461] bcache: register_bdev() registered backing device loop0
> [ 136.424464] bcache: bch_cached_dev_attach() Tried to attach loop0 but duplicate UUID already attached
>
> My test procedure is:
>
> dd if=/dev/sdb1 of=imgfile bs=1024 count=262144
> losetup -f imgfile
>
> Signed-off-by: Michael Lyle <mlyle@lyle.org>
> ---
> drivers/md/bcache/super.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 9c141a8aaacc..5cace6892958 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -963,6 +963,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
> uint32_t rtime = cpu_to_le32(get_seconds());
> struct uuid_entry *u;
> char buf[BDEVNAME_SIZE];
> + struct cached_dev *exist_dc, *t;
>
> bdevname(dc->bdev, buf);
>
> @@ -987,6 +988,16 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
> return -EINVAL;
> }
>
> + /* Check whether already attached */
> + list_for_each_entry_safe(exist_dc, t, &c->cached_devs, list) {
> + if (!memcmp(dc->sb.uuid, exist_dc->sb.uuid, 16)) {
> + pr_err("Tried to attach %s but duplicate UUID already attached",
> + buf);
> +
> + return -EINVAL;
> + }
> + }
> +
> u = uuid_find(c, dc->sb.uuid);
>
> if (u &&
> --
> 2.14.1
Thanks
Tang Junhui
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] bcache: don't attach backing with duplicate UUID
@ 2018-03-01 16:05 Michael Lyle
0 siblings, 0 replies; 6+ messages in thread
From: Michael Lyle @ 2018-03-01 16:05 UTC (permalink / raw)
Cc: linux-bcache, linux-block, Michael Lyle
This can happen e.g. during disk cloning.
This is an incomplete fix: it does not catch duplicate UUIDs earlier
when things are still unattached. It does not unregister the device.
Further changes to cope better with this are planned but conflict with
Coly's ongoing improvements to handling device errors. In the meantime,
one can manually stop the device after this has happened.
Attempts to attach a duplicate device result in:
[ 136.372404] loop: module loaded
[ 136.424461] bcache: register_bdev() registered backing device loop0
[ 136.424464] bcache: bch_cached_dev_attach() Tried to attach loop0 but duplicate UUID already attached
My test procedure is:
dd if=/dev/sdb1 of=imgfile bs=1024 count=262144
losetup -f imgfile
Signed-off-by: Michael Lyle <mlyle@lyle.org>
---
drivers/md/bcache/super.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 9c141a8aaacc..5cace6892958 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -963,6 +963,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
uint32_t rtime = cpu_to_le32(get_seconds());
struct uuid_entry *u;
char buf[BDEVNAME_SIZE];
+ struct cached_dev *exist_dc, *t;
bdevname(dc->bdev, buf);
@@ -987,6 +988,16 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
return -EINVAL;
}
+ /* Check whether already attached */
+ list_for_each_entry_safe(exist_dc, t, &c->cached_devs, list) {
+ if (!memcmp(dc->sb.uuid, exist_dc->sb.uuid, 16)) {
+ pr_err("Tried to attach %s but duplicate UUID already attached",
+ buf);
+
+ return -EINVAL;
+ }
+ }
+
u = uuid_find(c, dc->sb.uuid);
if (u &&
--
2.14.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-05 21:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-05 1:07 [PATCH] bcache: don't attach backing with duplicate UUID tang.junhui
2018-03-05 21:46 ` Michael Lyle
-- strict thread matches above, loose matches on Subject: below --
2018-03-02 5:45 tang.junhui
2018-03-02 14:55 ` Michael Lyle
2018-03-02 14:59 ` Michael Lyle
2018-03-01 16:05 Michael Lyle
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).