Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1380114 > unrolled thread

Re: [PATCH 17/19] dm: get rid of superfluous gfp flags

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 17/19] dm: get rid of superfluous gfp flags Mikulas Patocka <mpatocka@redhat.com> - 2016-04-15 20:50 +0200
    Re: [PATCH 17/19] dm: get rid of superfluous gfp flags Michal Hocko <mhocko@kernel.org> - 2016-04-16 22:40 +0200
      Re: [PATCH 17/19] dm: get rid of superfluous gfp flags Michal Hocko <mhocko@kernel.org> - 2016-04-22 14:50 +0200
        Re: [PATCH 17/19] dm: get rid of superfluous gfp flags Mikulas Patocka <mpatocka@redhat.com> - 2016-04-26 19:30 +0200
          Re: [PATCH 17/19] dm: get rid of superfluous gfp flags Michal Hocko <mhocko@kernel.org> - 2016-04-27 10:40 +0200

#1380114 — Re: [PATCH 17/19] dm: get rid of superfluous gfp flags

On Fri, 15 Apr 2016, Michal Hocko wrote:

> On Fri 15-04-16 08:29:28, Mikulas Patocka wrote:
> > 
> > 
> > On Mon, 11 Apr 2016, Michal Hocko wrote:
> > 
> > > From: Michal Hocko <mhocko@suse.com>
> > > 
> > > copy_params seems to be little bit confused about which allocation flags
> > > to use. It enforces GFP_NOIO even though it uses
> > > memalloc_noio_{save,restore} which enforces GFP_NOIO at the page
> > 
> > memalloc_noio_{save,restore} is used because __vmalloc is flawed and 
> > doesn't respect GFP_NOIO properly (it doesn't use gfp flags when 
> > allocating pagetables).
> 
> Yes and there are no plans to change __vmalloc to properly propagate gfp
> flags through the whole call chain and that is why we have
> memalloc_noio thingy. If that ever changes later the GFP_NOIO can be
> added in favor of memalloc_noio API. Both are clearly redundant.
> -- 
> Michal Hocko
> SUSE Labs

You could move memalloc_noio_{save,restore} to __vmalloc. Something like

if (!(gfp_mask & __GFP_IO))
	noio_flag = memalloc_noio_save();
...
if (!(gfp_mask & __GFP_IO))
	memalloc_noio_restore(noio_flag);

That would be better than repeating this hack in every __vmalloc caller 
that need GFP_NOIO.

Mikulas

[toc] | [next] | [standalone]

#1380619

On Fri 15-04-16 14:41:29, Mikulas Patocka wrote:
> 
> 
> On Fri, 15 Apr 2016, Michal Hocko wrote:
> 
> > On Fri 15-04-16 08:29:28, Mikulas Patocka wrote:
> > > 
> > > 
> > > On Mon, 11 Apr 2016, Michal Hocko wrote:
> > > 
> > > > From: Michal Hocko <mhocko@suse.com>
> > > > 
> > > > copy_params seems to be little bit confused about which allocation flags
> > > > to use. It enforces GFP_NOIO even though it uses
> > > > memalloc_noio_{save,restore} which enforces GFP_NOIO at the page
> > > 
> > > memalloc_noio_{save,restore} is used because __vmalloc is flawed and 
> > > doesn't respect GFP_NOIO properly (it doesn't use gfp flags when 
> > > allocating pagetables).
> > 
> > Yes and there are no plans to change __vmalloc to properly propagate gfp
> > flags through the whole call chain and that is why we have
> > memalloc_noio thingy. If that ever changes later the GFP_NOIO can be
> > added in favor of memalloc_noio API. Both are clearly redundant.
> > -- 
> > Michal Hocko
> > SUSE Labs
> 
> You could move memalloc_noio_{save,restore} to __vmalloc. Something like
> 
> if (!(gfp_mask & __GFP_IO))
> 	noio_flag = memalloc_noio_save();
> ...
> if (!(gfp_mask & __GFP_IO))
> 	memalloc_noio_restore(noio_flag);
> 
> That would be better than repeating this hack in every __vmalloc caller 
> that need GFP_NOIO.

It is not my intention to change __vmalloc behavior. If you strongly
oppose the GFP_NOIO change I can drop it from the patch. It is
__GFP_REPEAT which I am after.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1385129

On Sat 16-04-16 16:31:35, Michal Hocko wrote:
> On Fri 15-04-16 14:41:29, Mikulas Patocka wrote:
> > 
> > 
> > On Fri, 15 Apr 2016, Michal Hocko wrote:
> > 
> > > On Fri 15-04-16 08:29:28, Mikulas Patocka wrote:
> > > > 
> > > > 
> > > > On Mon, 11 Apr 2016, Michal Hocko wrote:
> > > > 
> > > > > From: Michal Hocko <mhocko@suse.com>
> > > > > 
> > > > > copy_params seems to be little bit confused about which allocation flags
> > > > > to use. It enforces GFP_NOIO even though it uses
> > > > > memalloc_noio_{save,restore} which enforces GFP_NOIO at the page
> > > > 
> > > > memalloc_noio_{save,restore} is used because __vmalloc is flawed and 
> > > > doesn't respect GFP_NOIO properly (it doesn't use gfp flags when 
> > > > allocating pagetables).
> > > 
> > > Yes and there are no plans to change __vmalloc to properly propagate gfp
> > > flags through the whole call chain and that is why we have
> > > memalloc_noio thingy. If that ever changes later the GFP_NOIO can be
> > > added in favor of memalloc_noio API. Both are clearly redundant.
> > > -- 
> > > Michal Hocko
> > > SUSE Labs
> > 
> > You could move memalloc_noio_{save,restore} to __vmalloc. Something like
> > 
> > if (!(gfp_mask & __GFP_IO))
> > 	noio_flag = memalloc_noio_save();
> > ...
> > if (!(gfp_mask & __GFP_IO))
> > 	memalloc_noio_restore(noio_flag);
> > 
> > That would be better than repeating this hack in every __vmalloc caller 
> > that need GFP_NOIO.
> 
> It is not my intention to change __vmalloc behavior. If you strongly
> oppose the GFP_NOIO change I can drop it from the patch. It is
> __GFP_REPEAT which I am after.

I am dropping the GFP_NOIO part for this patch but now that I am looking
into the code more closely I completely fail why it is needed in the
first place.

copy_params seems to be called only from the ioctl context which doesn't
hold any locks which would lockup during the direct reclaim AFAICS. The
git log shows that the code has used PF_MEMALLOC before which is even
bigger mystery to me. Could you please clarify why this is GFP_NOIO
restricted context? Maybe it needed to be in the past but I do not see
any reason for it to be now so unless I am missing something the
GFP_KERNEL should be perfectly OK. Also note that GFP_NOIO wouldn't work
properly because there are copy_from_user calls in the same path which
could page fault and do GFP_KERNEL allocations anyway. I can send follow
up cleanups unless I am missing something subtle here.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1387743

On Fri, 22 Apr 2016, Michal Hocko wrote:

> On Sat 16-04-16 16:31:35, Michal Hocko wrote:
> > On Fri 15-04-16 14:41:29, Mikulas Patocka wrote:
> > > 
> > > 
> > > On Fri, 15 Apr 2016, Michal Hocko wrote:
> > > 
> > > > On Fri 15-04-16 08:29:28, Mikulas Patocka wrote:
> > > > > 
> > > > > 
> > > > > On Mon, 11 Apr 2016, Michal Hocko wrote:
> > > > > 
> > > > > > From: Michal Hocko <mhocko@suse.com>
> > > > > > 
> > > > > > copy_params seems to be little bit confused about which allocation flags
> > > > > > to use. It enforces GFP_NOIO even though it uses
> > > > > > memalloc_noio_{save,restore} which enforces GFP_NOIO at the page
> > > > > 
> > > > > memalloc_noio_{save,restore} is used because __vmalloc is flawed and 
> > > > > doesn't respect GFP_NOIO properly (it doesn't use gfp flags when 
> > > > > allocating pagetables).
> > > > 
> > > > Yes and there are no plans to change __vmalloc to properly propagate gfp
> > > > flags through the whole call chain and that is why we have
> > > > memalloc_noio thingy. If that ever changes later the GFP_NOIO can be
> > > > added in favor of memalloc_noio API. Both are clearly redundant.
> > > > -- 
> > > > Michal Hocko
> > > > SUSE Labs
> > > 
> > > You could move memalloc_noio_{save,restore} to __vmalloc. Something like
> > > 
> > > if (!(gfp_mask & __GFP_IO))
> > > 	noio_flag = memalloc_noio_save();
> > > ...
> > > if (!(gfp_mask & __GFP_IO))
> > > 	memalloc_noio_restore(noio_flag);
> > > 
> > > That would be better than repeating this hack in every __vmalloc caller 
> > > that need GFP_NOIO.
> > 
> > It is not my intention to change __vmalloc behavior. If you strongly
> > oppose the GFP_NOIO change I can drop it from the patch. It is
> > __GFP_REPEAT which I am after.
> 
> I am dropping the GFP_NOIO part for this patch but now that I am looking
> into the code more closely I completely fail why it is needed in the
> first place.
> 
> copy_params seems to be called only from the ioctl context which doesn't
> hold any locks which would lockup during the direct reclaim AFAICS. The
> git log shows that the code has used PF_MEMALLOC before which is even
> bigger mystery to me. Could you please clarify why this is GFP_NOIO
> restricted context? Maybe it needed to be in the past but I do not see
> any reason for it to be now so unless I am missing something the
> GFP_KERNEL should be perfectly OK. Also note that GFP_NOIO wouldn't work
> properly because there are copy_from_user calls in the same path which
> could page fault and do GFP_KERNEL allocations anyway. I can send follow
> up cleanups unless I am missing something subtle here.
> -- 
> Michal Hocko
> SUSE Labs

The LVM tool calls suspend and resume ioctls on device mapper block 
devices.

When a device is suspended, any bio sent to the device is held. If the 
resume ioctl did GFP_KERNEL allocation, the allocation could get stuck 
trying to write some dirty cached pages to the suspended device.

The LVM tool and the dmeventd daemon use mlock to lock its address space, 
so the copy_from_user/copy_to_user call cannot trigger a page fault.

Mikulas

[toc] | [prev] | [next] | [standalone]

#1388784

[Adding dm-devel@redhat.com to CC]

On Tue 26-04-16 13:20:04, Mikulas Patocka wrote:
> On Fri, 22 Apr 2016, Michal Hocko wrote:
[...]
> > copy_params seems to be called only from the ioctl context which doesn't
> > hold any locks which would lockup during the direct reclaim AFAICS. The
> > git log shows that the code has used PF_MEMALLOC before which is even
> > bigger mystery to me. Could you please clarify why this is GFP_NOIO
> > restricted context? Maybe it needed to be in the past but I do not see
> > any reason for it to be now so unless I am missing something the
> > GFP_KERNEL should be perfectly OK. Also note that GFP_NOIO wouldn't work
> > properly because there are copy_from_user calls in the same path which
> > could page fault and do GFP_KERNEL allocations anyway. I can send follow
> > up cleanups unless I am missing something subtle here.
> 
> The LVM tool calls suspend and resume ioctls on device mapper block 
> devices.
>
> When a device is suspended, any bio sent to the device is held. If the 
> resume ioctl did GFP_KERNEL allocation, the allocation could get stuck 
> trying to write some dirty cached pages to the suspended device.
> 
> The LVM tool and the dmeventd daemon use mlock to lock its address space, 
> so the copy_from_user/copy_to_user call cannot trigger a page fault.

OK, I see, thanks for the clarification! This sounds fragile to me
though. Wouldn't it be better to use the memalloc_noio_save for the
whole copy_params instead? That would force all possible allocations to
not trigger any IO. Something like the following.
---
From dbb2338bb88d2da1ff24cee59cbffd120b119e3b Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Wed, 27 Apr 2016 10:26:13 +0200
Subject: [PATCH] dm: clean up GFP_NIO usage

copy_params uses GFP_NOIO for explicit allocation requests because this
might be called from the suspend path. To quote Mikulas:
: The LVM tool calls suspend and resume ioctls on device mapper block
: devices.
:
: When a device is suspended, any bio sent to the device is held. If the
: resume ioctl did GFP_KERNEL allocation, the allocation could get stuck
: trying to write some dirty cached pages to the suspended device.
:
: The LVM tool and the dmeventd daemon use mlock to lock its address space,
: so the copy_from_user/copy_to_user call cannot trigger a page fault.

Relying on the mlock is quite fragile and we have a better way in kernel
to enfore NOIO which is already used for the vmalloc fallback. Just use
memalloc_noio_{save,restore} around the whole copy_params function which
will force the same also to the page fult paths via copy_{from,to}_user.

While we are there we can also remove __GFP_NOMEMALLOC because copy_params
is never called from MEMALLOC context (e.g. during the reclaim).

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 drivers/md/dm-ioctl.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 2c7ca258c4e4..fe0b57d7573c 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1715,16 +1715,13 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
 	 */
 	dmi = NULL;
 	if (param_kernel->data_size <= KMALLOC_MAX_SIZE) {
-		dmi = kmalloc(param_kernel->data_size, GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
+		dmi = kmalloc(param_kernel->data_size, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
 		if (dmi)
 			*param_flags |= DM_PARAMS_KMALLOC;
 	}
 
 	if (!dmi) {
-		unsigned noio_flag;
-		noio_flag = memalloc_noio_save();
-		dmi = __vmalloc(param_kernel->data_size, GFP_NOIO | __GFP_HIGH | __GFP_HIGHMEM, PAGE_KERNEL);
-		memalloc_noio_restore(noio_flag);
+		dmi = __vmalloc(param_kernel->data_size, GFP_KERNEL | __GFP_HIGH | __GFP_HIGHMEM, PAGE_KERNEL);
 		if (dmi)
 			*param_flags |= DM_PARAMS_VMALLOC;
 	}
@@ -1801,6 +1798,7 @@ static int ctl_ioctl(uint command, struct dm_ioctl __user *user)
 	ioctl_fn fn = NULL;
 	size_t input_param_size;
 	struct dm_ioctl param_kernel;
+	unsigned noio_flag;
 
 	/* only root can play with this */
 	if (!capable(CAP_SYS_ADMIN))
@@ -1832,9 +1830,12 @@ static int ctl_ioctl(uint command, struct dm_ioctl __user *user)
 	}
 
 	/*
-	 * Copy the parameters into kernel space.
+	 * Copy the parameters into kernel space. Make sure that no IO is triggered
+	 * from the allocation paths because this might be called during the suspend.
 	 */
+	noio_flag = memalloc_noio_save();
 	r = copy_params(user, &param_kernel, ioctl_flags, &param, &param_flags);
+	memalloc_noio_restore(noio_flag);
 
 	if (r)
 		return r;
-- 
2.8.0.rc3

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web