From: Toshi Kani <toshi.kani@hpe.com> To: Vishal Verma <vishal@kernel.org>, dan.j.williams@intel.com Cc: linux-kernel@vger.kernel.org, micah.parrish@hpe.com, linux-nvdimm@lists.01.org Subject: Re: [PATCH] BTT: Change nd_btt_arena_is_valid() to verify UUID Date: Fri, 18 Dec 2015 08:15:37 -0700 [thread overview] Message-ID: <1450451737.20148.121.camel@hpe.com> (raw) In-Reply-To: <1450427687.11503.14.camel@kernel.org> On Fri, 2015-12-18 at 01:34 -0700, Vishal Verma wrote: > On Thu, 2015-12-17 at 16:00 -0700, Toshi Kani wrote: > > When user unbinds a BTT disk and binds again with a different > > sector size without wiping out the disk, a BTT disk is created > > with a wrong size. > > I think this is an incorrect usage model in the first place. You > shouldn't expect to disable a BTT, change the sector size or uuid > behind it, and expect it to work with the new sector size on re- > enabling. Well, users do test with multiple sector sizes when they evaluate BTT. So, this is a legitimate use-case with a missing step to wipe out the data in between. I believe BTT needs to be protected from this case in one way or the other. > While this patch makes the BTT see the right size, it is really just an > illusion, because if you try to read the pre-sector-size-change data, > it will be scrambled, and thus practically lost. When user binds a new BTT, he/she does not expect any valid data left in BTT. mkfs is then performed before using the BTT disk. This use-case is similar with re-partitioning. > Even with this patch, I can skip changing the UUID, just change the > sector size, and re-enable it, and the total available size will appear > to have changed. Using the same UUID requires deliberate effort since uuidgen won't generate the same UUID again. We can check the sector size in addition to the UUID, if that makes it any safer. I am not sure how much we need to protect from such deliberate effort, though. > For the case of legacy (non-nfit) namespaces, the only way to change a > BTT's properties is to recreate it using 'force_raw'. For non-legacy > namespaces, the recommended way is to recreate the namespace with a new > uuid, and this will cause BTT to react to the parent_uuid change and > not try to bind itself to stale metadata. Both cases will lose any data > on the old BTT. Losing the data is expected after newly binding BTT, which is not an issue here. The issue is that BTT operates in split sector sizes when user missed the step to wipe out the data before binding. Yes, this patch follows the same approach of the parent_uuid check. > Ideally, changing BTT properties shouldn't be allowed till the parent > namespaces is recreated, but I'm not sure there is an easy way to > enforce this -- Dan? > > Also, I wonder if this problem is solved by using libndctl to manage > BTTs. I have not tested with libndctl yet, but I think our bind/unbind scripts do the same procedures. Thanks, -Toshi
WARNING: multiple messages have this Message-ID (diff)
From: Toshi Kani <toshi.kani@hpe.com> To: Vishal Verma <vishal@kernel.org>, dan.j.williams@intel.com Cc: linux-kernel@vger.kernel.org, micah.parrish@hpe.com, linux-nvdimm@ml01.01.org Subject: Re: [PATCH] BTT: Change nd_btt_arena_is_valid() to verify UUID Date: Fri, 18 Dec 2015 08:15:37 -0700 [thread overview] Message-ID: <1450451737.20148.121.camel@hpe.com> (raw) In-Reply-To: <1450427687.11503.14.camel@kernel.org> On Fri, 2015-12-18 at 01:34 -0700, Vishal Verma wrote: > On Thu, 2015-12-17 at 16:00 -0700, Toshi Kani wrote: > > When user unbinds a BTT disk and binds again with a different > > sector size without wiping out the disk, a BTT disk is created > > with a wrong size. > > I think this is an incorrect usage model in the first place. You > shouldn't expect to disable a BTT, change the sector size or uuid > behind it, and expect it to work with the new sector size on re- > enabling. Well, users do test with multiple sector sizes when they evaluate BTT. So, this is a legitimate use-case with a missing step to wipe out the data in between. I believe BTT needs to be protected from this case in one way or the other. > While this patch makes the BTT see the right size, it is really just an > illusion, because if you try to read the pre-sector-size-change data, > it will be scrambled, and thus practically lost. When user binds a new BTT, he/she does not expect any valid data left in BTT. mkfs is then performed before using the BTT disk. This use-case is similar with re-partitioning. > Even with this patch, I can skip changing the UUID, just change the > sector size, and re-enable it, and the total available size will appear > to have changed. Using the same UUID requires deliberate effort since uuidgen won't generate the same UUID again. We can check the sector size in addition to the UUID, if that makes it any safer. I am not sure how much we need to protect from such deliberate effort, though. > For the case of legacy (non-nfit) namespaces, the only way to change a > BTT's properties is to recreate it using 'force_raw'. For non-legacy > namespaces, the recommended way is to recreate the namespace with a new > uuid, and this will cause BTT to react to the parent_uuid change and > not try to bind itself to stale metadata. Both cases will lose any data > on the old BTT. Losing the data is expected after newly binding BTT, which is not an issue here. The issue is that BTT operates in split sector sizes when user missed the step to wipe out the data before binding. Yes, this patch follows the same approach of the parent_uuid check. > Ideally, changing BTT properties shouldn't be allowed till the parent > namespaces is recreated, but I'm not sure there is an easy way to > enforce this -- Dan? > > Also, I wonder if this problem is solved by using libndctl to manage > BTTs. I have not tested with libndctl yet, but I think our bind/unbind scripts do the same procedures. Thanks, -Toshi
next prev parent reply other threads:[~2015-12-18 15:15 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-12-17 23:00 [PATCH] BTT: Change nd_btt_arena_is_valid() to verify UUID Toshi Kani 2015-12-17 23:00 ` Toshi Kani 2015-12-18 8:34 ` Vishal Verma 2015-12-18 8:34 ` Vishal Verma 2015-12-18 15:15 ` Toshi Kani [this message] 2015-12-18 15:15 ` Toshi Kani 2015-12-18 17:54 ` Dan Williams 2015-12-18 17:54 ` Dan Williams 2015-12-18 18:52 ` Toshi Kani 2015-12-18 18:52 ` Toshi Kani 2015-12-18 22:23 ` Dan Williams 2015-12-18 22:23 ` Dan Williams 2016-01-12 15:02 ` Toshi Kani 2016-01-12 15:02 ` Toshi Kani 2016-01-12 16:38 ` Dan Williams 2016-01-12 16:38 ` Dan Williams 2016-01-12 16:47 ` Linda Knippers 2016-01-12 16:47 ` Linda Knippers 2016-01-12 16:51 ` Dan Williams 2016-01-12 16:51 ` Dan Williams
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1450451737.20148.121.camel@hpe.com \ --to=toshi.kani@hpe.com \ --cc=dan.j.williams@intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nvdimm@lists.01.org \ --cc=micah.parrish@hpe.com \ --cc=vishal@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.