| Message ID | 1434885634-19895-3-git-send-email-pali.rohar@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Sun, Jun 21 2015 at 7:20am -0400, Pali Rohár <pali.rohar@gmail.com> wrote: > This patch exports function dm_suspend_md() which suspend mapped device so other > kernel drivers can use it and could suspend mapped device when needed. > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > --- > drivers/md/dm.c | 6 ++++++ > drivers/md/dm.h | 5 +++++ > 2 files changed, 11 insertions(+) > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > index 2caf492..03298ff 100644 > --- a/drivers/md/dm.c > +++ b/drivers/md/dm.c > @@ -3343,6 +3343,12 @@ out: > return r; > } > > +int dm_suspend_md(struct mapped_device *md) > +{ > + return dm_suspend(md, DM_SUSPEND_LOCKFS_FLAG); > +} > +EXPORT_SYMBOL_GPL(dm_suspend_md); > + > /* > * Internal suspend/resume works like userspace-driven suspend. It waits > * until all bios finish and prevents issuing new bios to the target drivers. To do this properly you should be introducing a variant of dm_internal_suspend. We currently have two variants: dm_internal_suspend_fast dm_internal_suspend_noflush The reason to use a dm_internal_suspend variant is this suspend was _not_ initiated by an upper level ioctl (from userspace). It was done internally from within the target. You're explicitly using DM_SUSPEND_LOCKFS_FLAG above.. meaning you're interested in flushing all pending IO (in the FS layered on dm-crupt, if one exists). But see the comment in __dm_internal_suspend() about TASK_UNINTERRUPTIBLE. If you're OK with the dm-crypt initiated suspend being TASK_UNINTERRUPTIBLE then you could just introduce: void dm_internal_suspend_uninterruptible_flush(struct mapped_device *md) { mutex_lock(&md->suspend_lock); __dm_internal_suspend(md, DM_SUSPEND_LOCKFS_FLAG); mutex_unlock(&md->suspend_lock); } EXPORT_SYMBOL_GPL(dm_internal_suspend_uninterruptible_flush); Otherwise, there is much more extensive DM core changes needed to __dm_internal_suspend() and .presuspend to properly support TASK_INTERRUPTIBLE. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 17 July 2015 10:04:39 Mike Snitzer wrote: > On Sun, Jun 21 2015 at 7:20am -0400, > Pali Rohár <pali.rohar@gmail.com> wrote: > > > This patch exports function dm_suspend_md() which suspend mapped device so other > > kernel drivers can use it and could suspend mapped device when needed. > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > --- > > drivers/md/dm.c | 6 ++++++ > > drivers/md/dm.h | 5 +++++ > > 2 files changed, 11 insertions(+) > > > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > > index 2caf492..03298ff 100644 > > --- a/drivers/md/dm.c > > +++ b/drivers/md/dm.c > > @@ -3343,6 +3343,12 @@ out: > > return r; > > } > > > > +int dm_suspend_md(struct mapped_device *md) > > +{ > > + return dm_suspend(md, DM_SUSPEND_LOCKFS_FLAG); > > +} > > +EXPORT_SYMBOL_GPL(dm_suspend_md); > > + > > /* > > * Internal suspend/resume works like userspace-driven suspend. It waits > > * until all bios finish and prevents issuing new bios to the target drivers. > > To do this properly you should be introducing a variant of > dm_internal_suspend. We currently have two variants: > dm_internal_suspend_fast > dm_internal_suspend_noflush > > The reason to use a dm_internal_suspend variant is this suspend was > _not_ initiated by an upper level ioctl (from userspace). It was > done internally from within the target. > > You're explicitly using DM_SUSPEND_LOCKFS_FLAG above.. meaning you're > interested in flushing all pending IO (in the FS layered on dm-crupt, if > one exists). > > But see the comment in __dm_internal_suspend() about TASK_UNINTERRUPTIBLE. > If you're OK with the dm-crypt initiated suspend being > TASK_UNINTERRUPTIBLE then you could just introduce: > > void dm_internal_suspend_uninterruptible_flush(struct mapped_device *md) > { > mutex_lock(&md->suspend_lock); > __dm_internal_suspend(md, DM_SUSPEND_LOCKFS_FLAG); > mutex_unlock(&md->suspend_lock); > } > EXPORT_SYMBOL_GPL(dm_internal_suspend_uninterruptible_flush); > > Otherwise, there is much more extensive DM core changes needed to > __dm_internal_suspend() and .presuspend to properly support > TASK_INTERRUPTIBLE. Hi! I will look at dm_internal_suspend. Anyway use case for suspend is from dm-crypt to do both operations: suspend + key wipe. It means that without entering key again from userspace, resume is not possible. So my question is: It is possible to do internal suspend and then using resume from userspace via ioctl?
On Fri, Jul 17 2015 at 10:22am -0400, Pali Rohár <pali.rohar@gmail.com> wrote: > On Friday 17 July 2015 10:04:39 Mike Snitzer wrote: > > On Sun, Jun 21 2015 at 7:20am -0400, > > Pali Rohár <pali.rohar@gmail.com> wrote: > > > > > This patch exports function dm_suspend_md() which suspend mapped device so other > > > kernel drivers can use it and could suspend mapped device when needed. > > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > > --- > > > drivers/md/dm.c | 6 ++++++ > > > drivers/md/dm.h | 5 +++++ > > > 2 files changed, 11 insertions(+) > > > > > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > > > index 2caf492..03298ff 100644 > > > --- a/drivers/md/dm.c > > > +++ b/drivers/md/dm.c > > > @@ -3343,6 +3343,12 @@ out: > > > return r; > > > } > > > > > > +int dm_suspend_md(struct mapped_device *md) > > > +{ > > > + return dm_suspend(md, DM_SUSPEND_LOCKFS_FLAG); > > > +} > > > +EXPORT_SYMBOL_GPL(dm_suspend_md); > > > + > > > /* > > > * Internal suspend/resume works like userspace-driven suspend. It waits > > > * until all bios finish and prevents issuing new bios to the target drivers. > > > > To do this properly you should be introducing a variant of > > dm_internal_suspend. We currently have two variants: > > dm_internal_suspend_fast > > dm_internal_suspend_noflush > > > > The reason to use a dm_internal_suspend variant is this suspend was > > _not_ initiated by an upper level ioctl (from userspace). It was > > done internally from within the target. > > > > You're explicitly using DM_SUSPEND_LOCKFS_FLAG above.. meaning you're > > interested in flushing all pending IO (in the FS layered on dm-crupt, if > > one exists). > > > > But see the comment in __dm_internal_suspend() about TASK_UNINTERRUPTIBLE. > > If you're OK with the dm-crypt initiated suspend being > > TASK_UNINTERRUPTIBLE then you could just introduce: > > > > void dm_internal_suspend_uninterruptible_flush(struct mapped_device *md) > > { > > mutex_lock(&md->suspend_lock); > > __dm_internal_suspend(md, DM_SUSPEND_LOCKFS_FLAG); > > mutex_unlock(&md->suspend_lock); > > } > > EXPORT_SYMBOL_GPL(dm_internal_suspend_uninterruptible_flush); > > > > Otherwise, there is much more extensive DM core changes needed to > > __dm_internal_suspend() and .presuspend to properly support > > TASK_INTERRUPTIBLE. > > Hi! I will look at dm_internal_suspend. Anyway use case for suspend is > from dm-crypt to do both operations: suspend + key wipe. It means that > without entering key again from userspace, resume is not possible. So my > question is: It is possible to do internal suspend and then using resume > from userspace via ioctl? Good question: no, userspace resume would block waiting for internal resume. Soooo... I'll have to look at your patch 3 to understand why dm-crypt is needing to initiate the suspend internally but then userspace is initiating the resume... this imbalance is concerning. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 17 2015 at 11:22am -0400, Mike Snitzer <snitzer@redhat.com> wrote: > On Fri, Jul 17 2015 at 10:22am -0400, > Pali Rohár <pali.rohar@gmail.com> wrote: > > > On Friday 17 July 2015 10:04:39 Mike Snitzer wrote: > > > On Sun, Jun 21 2015 at 7:20am -0400, > > > Pali Rohár <pali.rohar@gmail.com> wrote: > > > > > > > This patch exports function dm_suspend_md() which suspend mapped device so other > > > > kernel drivers can use it and could suspend mapped device when needed. > > > > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > > > --- > > > > drivers/md/dm.c | 6 ++++++ > > > > drivers/md/dm.h | 5 +++++ > > > > 2 files changed, 11 insertions(+) > > > > > > > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > > > > index 2caf492..03298ff 100644 > > > > --- a/drivers/md/dm.c > > > > +++ b/drivers/md/dm.c > > > > @@ -3343,6 +3343,12 @@ out: > > > > return r; > > > > } > > > > > > > > +int dm_suspend_md(struct mapped_device *md) > > > > +{ > > > > + return dm_suspend(md, DM_SUSPEND_LOCKFS_FLAG); > > > > +} > > > > +EXPORT_SYMBOL_GPL(dm_suspend_md); > > > > + > > > > /* > > > > * Internal suspend/resume works like userspace-driven suspend. It waits > > > > * until all bios finish and prevents issuing new bios to the target drivers. > > > > > > To do this properly you should be introducing a variant of > > > dm_internal_suspend. We currently have two variants: > > > dm_internal_suspend_fast > > > dm_internal_suspend_noflush > > > > > > The reason to use a dm_internal_suspend variant is this suspend was > > > _not_ initiated by an upper level ioctl (from userspace). It was > > > done internally from within the target. > > > > > > You're explicitly using DM_SUSPEND_LOCKFS_FLAG above.. meaning you're > > > interested in flushing all pending IO (in the FS layered on dm-crupt, if > > > one exists). > > > > > > But see the comment in __dm_internal_suspend() about TASK_UNINTERRUPTIBLE. > > > If you're OK with the dm-crypt initiated suspend being > > > TASK_UNINTERRUPTIBLE then you could just introduce: > > > > > > void dm_internal_suspend_uninterruptible_flush(struct mapped_device *md) > > > { > > > mutex_lock(&md->suspend_lock); > > > __dm_internal_suspend(md, DM_SUSPEND_LOCKFS_FLAG); > > > mutex_unlock(&md->suspend_lock); > > > } > > > EXPORT_SYMBOL_GPL(dm_internal_suspend_uninterruptible_flush); > > > > > > Otherwise, there is much more extensive DM core changes needed to > > > __dm_internal_suspend() and .presuspend to properly support > > > TASK_INTERRUPTIBLE. > > > > Hi! I will look at dm_internal_suspend. Anyway use case for suspend is > > from dm-crypt to do both operations: suspend + key wipe. It means that > > without entering key again from userspace, resume is not possible. So my > > question is: It is possible to do internal suspend and then using resume > > from userspace via ioctl? > > Good question: no, userspace resume would block waiting for internal > resume. > > Soooo... I'll have to look at your patch 3 to understand why dm-crypt is > needing to initiate the suspend internally but then userspace is > initiating the resume... this imbalance is concerning. Why not introduce a new message that allows you to wipe the key after suspend? Both initiated from userspace. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 17 July 2015 17:30:45 Mike Snitzer wrote: > On Fri, Jul 17 2015 at 11:22am -0400, > > Mike Snitzer <snitzer@redhat.com> wrote: > > On Fri, Jul 17 2015 at 10:22am -0400, > > > > Pali Rohár <pali.rohar@gmail.com> wrote: > > > On Friday 17 July 2015 10:04:39 Mike Snitzer wrote: > > > > On Sun, Jun 21 2015 at 7:20am -0400, > > > > > > > > Pali Rohár <pali.rohar@gmail.com> wrote: > > > > > This patch exports function dm_suspend_md() which suspend > > > > > mapped device so other kernel drivers can use it and could > > > > > suspend mapped device when needed. > > > > > > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > > > > --- > > > > > > > > > > drivers/md/dm.c | 6 ++++++ > > > > > drivers/md/dm.h | 5 +++++ > > > > > 2 files changed, 11 insertions(+) > > > > > > > > > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > > > > > index 2caf492..03298ff 100644 > > > > > --- a/drivers/md/dm.c > > > > > +++ b/drivers/md/dm.c > > > > > > > > > > @@ -3343,6 +3343,12 @@ out: > > > > > return r; > > > > > > > > > > } > > > > > > > > > > +int dm_suspend_md(struct mapped_device *md) > > > > > +{ > > > > > + return dm_suspend(md, DM_SUSPEND_LOCKFS_FLAG); > > > > > +} > > > > > +EXPORT_SYMBOL_GPL(dm_suspend_md); > > > > > + > > > > > > > > > > /* > > > > > > > > > > * Internal suspend/resume works like userspace-driven > > > > > suspend. It waits * until all bios finish and prevents > > > > > issuing new bios to the target drivers. > > > > > > > > To do this properly you should be introducing a variant of > > > > dm_internal_suspend. We currently have two variants: > > > > dm_internal_suspend_fast > > > > dm_internal_suspend_noflush > > > > > > > > The reason to use a dm_internal_suspend variant is this suspend > > > > was _not_ initiated by an upper level ioctl (from userspace). > > > > It was done internally from within the target. > > > > > > > > You're explicitly using DM_SUSPEND_LOCKFS_FLAG above.. meaning > > > > you're interested in flushing all pending IO (in the FS > > > > layered on dm-crupt, if one exists). > > > > > > > > But see the comment in __dm_internal_suspend() about > > > > TASK_UNINTERRUPTIBLE. If you're OK with the dm-crypt initiated > > > > suspend being TASK_UNINTERRUPTIBLE then you could just > > > > introduce: > > > > > > > > void dm_internal_suspend_uninterruptible_flush(struct > > > > mapped_device *md) { > > > > > > > > mutex_lock(&md->suspend_lock); > > > > __dm_internal_suspend(md, DM_SUSPEND_LOCKFS_FLAG); > > > > mutex_unlock(&md->suspend_lock); > > > > > > > > } > > > > EXPORT_SYMBOL_GPL(dm_internal_suspend_uninterruptible_flush); > > > > > > > > Otherwise, there is much more extensive DM core changes needed > > > > to __dm_internal_suspend() and .presuspend to properly support > > > > TASK_INTERRUPTIBLE. > > > > > > Hi! I will look at dm_internal_suspend. Anyway use case for > > > suspend is from dm-crypt to do both operations: suspend + key > > > wipe. It means that without entering key again from userspace, > > > resume is not possible. So my question is: It is possible to do > > > internal suspend and then using resume from userspace via ioctl? > > > > Good question: no, userspace resume would block waiting for > > internal resume. > > > > Soooo... I'll have to look at your patch 3 to understand why > > dm-crypt is needing to initiate the suspend internally but then > > userspace is initiating the resume... this imbalance is > > concerning. > > Why not introduce a new message that allows you to wipe the key after > suspend? Both initiated from userspace. There is already message for wiping key and it will success only if dm is suspended. But this patch series is fixing another problem: wipe key before suspend/hibernation action happend and to have it race free it must be done after userspace is freezed!
On Fri, Jul 17 2015 at 1:13pm -0400, Pali Rohár <pali.rohar@gmail.com> wrote: > On Friday 17 July 2015 17:30:45 Mike Snitzer wrote: > > On Fri, Jul 17 2015 at 11:22am -0400, > > > > Mike Snitzer <snitzer@redhat.com> wrote: > > > On Fri, Jul 17 2015 at 10:22am -0400, > > > > > > Pali Rohár <pali.rohar@gmail.com> wrote: > > > > On Friday 17 July 2015 10:04:39 Mike Snitzer wrote: > > > > > On Sun, Jun 21 2015 at 7:20am -0400, > > > > > > > > > > Pali Rohár <pali.rohar@gmail.com> wrote: > > > > > > This patch exports function dm_suspend_md() which suspend > > > > > > mapped device so other kernel drivers can use it and could > > > > > > suspend mapped device when needed. > > > > > > > > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > > > > > --- > > > > > > > > > > > > drivers/md/dm.c | 6 ++++++ > > > > > > drivers/md/dm.h | 5 +++++ > > > > > > 2 files changed, 11 insertions(+) > > > > > > > > > > > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > > > > > > index 2caf492..03298ff 100644 > > > > > > --- a/drivers/md/dm.c > > > > > > +++ b/drivers/md/dm.c > > > > > > > > > > > > @@ -3343,6 +3343,12 @@ out: > > > > > > return r; > > > > > > > > > > > > } > > > > > > > > > > > > +int dm_suspend_md(struct mapped_device *md) > > > > > > +{ > > > > > > + return dm_suspend(md, DM_SUSPEND_LOCKFS_FLAG); > > > > > > +} > > > > > > +EXPORT_SYMBOL_GPL(dm_suspend_md); > > > > > > + > > > > > > > > > > > > /* > > > > > > > > > > > > * Internal suspend/resume works like userspace-driven > > > > > > suspend. It waits * until all bios finish and prevents > > > > > > issuing new bios to the target drivers. > > > > > > > > > > To do this properly you should be introducing a variant of > > > > > dm_internal_suspend. We currently have two variants: > > > > > dm_internal_suspend_fast > > > > > dm_internal_suspend_noflush > > > > > > > > > > The reason to use a dm_internal_suspend variant is this suspend > > > > > was _not_ initiated by an upper level ioctl (from userspace). > > > > > It was done internally from within the target. > > > > > > > > > > You're explicitly using DM_SUSPEND_LOCKFS_FLAG above.. meaning > > > > > you're interested in flushing all pending IO (in the FS > > > > > layered on dm-crupt, if one exists). > > > > > > > > > > But see the comment in __dm_internal_suspend() about > > > > > TASK_UNINTERRUPTIBLE. If you're OK with the dm-crypt initiated > > > > > suspend being TASK_UNINTERRUPTIBLE then you could just > > > > > introduce: > > > > > > > > > > void dm_internal_suspend_uninterruptible_flush(struct > > > > > mapped_device *md) { > > > > > > > > > > mutex_lock(&md->suspend_lock); > > > > > __dm_internal_suspend(md, DM_SUSPEND_LOCKFS_FLAG); > > > > > mutex_unlock(&md->suspend_lock); > > > > > > > > > > } > > > > > EXPORT_SYMBOL_GPL(dm_internal_suspend_uninterruptible_flush); > > > > > > > > > > Otherwise, there is much more extensive DM core changes needed > > > > > to __dm_internal_suspend() and .presuspend to properly support > > > > > TASK_INTERRUPTIBLE. > > > > > > > > Hi! I will look at dm_internal_suspend. Anyway use case for > > > > suspend is from dm-crypt to do both operations: suspend + key > > > > wipe. It means that without entering key again from userspace, > > > > resume is not possible. So my question is: It is possible to do > > > > internal suspend and then using resume from userspace via ioctl? > > > > > > Good question: no, userspace resume would block waiting for > > > internal resume. > > > > > > Soooo... I'll have to look at your patch 3 to understand why > > > dm-crypt is needing to initiate the suspend internally but then > > > userspace is initiating the resume... this imbalance is > > > concerning. > > > > Why not introduce a new message that allows you to wipe the key after > > suspend? Both initiated from userspace. > > There is already message for wiping key and it will success only if dm > is suspended. > > But this patch series is fixing another problem: wipe key before > suspend/hibernation action happend and to have it race free it must be > done after userspace is freezed! Yes, I remember now. So it isn't even userspace initiating the suspend_and_wipe, it is the PM chain notifier code you're adding. I'll think more about your use of dm_suspend() -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 2caf492..03298ff 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -3343,6 +3343,12 @@ out: return r; } +int dm_suspend_md(struct mapped_device *md) +{ + return dm_suspend(md, DM_SUSPEND_LOCKFS_FLAG); +} +EXPORT_SYMBOL_GPL(dm_suspend_md); + /* * Internal suspend/resume works like userspace-driven suspend. It waits * until all bios finish and prevents issuing new bios to the target drivers. diff --git a/drivers/md/dm.h b/drivers/md/dm.h index 6123c2b..528e5e0 100644 --- a/drivers/md/dm.h +++ b/drivers/md/dm.h @@ -151,6 +151,11 @@ int dm_test_deferred_remove_flag(struct mapped_device *md); void dm_deferred_remove(void); /* + * Suspend mapped_device + */ +int dm_suspend_md(struct mapped_device *md); + +/* * The device-mapper can be driven through one of two interfaces; * ioctl or filesystem, depending which patch you have applied. */
This patch exports function dm_suspend_md() which suspend mapped device so other kernel drivers can use it and could suspend mapped device when needed. Signed-off-by: Pali Rohár <pali.rohar@gmail.com> --- drivers/md/dm.c | 6 ++++++ drivers/md/dm.h | 5 +++++ 2 files changed, 11 insertions(+)