From patchwork Wed Apr 27 08:35:30 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michal Hocko X-Patchwork-Id: 8953771 Return-Path: X-Original-To: patchwork-dm-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id E5889BF29F for ; Wed, 27 Apr 2016 08:48:15 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id E47A820225 for ; Wed, 27 Apr 2016 08:48:14 +0000 (UTC) Received: from mx6-phx2.redhat.com (mx6-phx2.redhat.com [209.132.183.39]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D1D85200F0 for ; Wed, 27 Apr 2016 08:48:13 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by mx6-phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u3R8iUk2061445; Wed, 27 Apr 2016 04:44:30 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id u3R8Zcqr027245 for ; Wed, 27 Apr 2016 04:35:38 -0400 Received: from mx1.redhat.com (ext-mx07.extmail.prod.ext.phx2.redhat.com [10.5.110.31]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u3R8ZcGg029424 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 27 Apr 2016 04:35:38 -0400 Received: from mail-wm0-f65.google.com (mail-wm0-f65.google.com [74.125.82.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B2CF9C049D7F; Wed, 27 Apr 2016 08:35:32 +0000 (UTC) Received: by mail-wm0-f65.google.com with SMTP id r12so10692609wme.0; Wed, 27 Apr 2016 01:35:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=R7Ega5jyo217RdU+CFwcKGiqPeOgnlKAsDX5f3DVExU=; b=fuOyvdC75ip7YkVyuMZvHLIJtTDfuMI7IQ2KZ4sDxOW2gVbiF5GZiEcY5cD/UeOEvt wEQBex3c/nCY1dCFZqxt1Foi0EJa9kr0cPzJPGvzMipysj5iEJaFrvQiSQMxbt7Sptzw pEkuvEVsQUqylU3i4KeUxbROqUdbvP3HvfT76MQAIxIu5eQ/heIjRbZnn1QktWHMWdNB KtLNJsa91tuAKIlSmAv9QZSOqqkuLMqy6rU1MlNAVE7xcZb4nUro9CRKpAjc+cmXHIUZ ZcvSth4VHSDtef4I9RIyclDbrJXn1SB3Rj1SBuDQqV3u19H6wZNZNkKfaBvjHU/N5Wj8 eeCw== X-Gm-Message-State: AOPr4FUix9CcRzwAACbD7kQjPPbC069dUZltUf8r+3CqXDIo11vaetxQkATt+mHgTaFg1Q== X-Received: by 10.28.126.145 with SMTP id z139mr22535028wmc.81.1461746131475; Wed, 27 Apr 2016 01:35:31 -0700 (PDT) Received: from localhost (nat1.scz.suse.com. [213.151.88.250]) by smtp.gmail.com with ESMTPSA id u4sm2828921wjz.4.2016.04.27.01.35.30 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 27 Apr 2016 01:35:30 -0700 (PDT) Date: Wed, 27 Apr 2016 10:35:30 +0200 From: Michal Hocko To: Mikulas Patocka Message-ID: <20160427083530.GD2179@dhcp22.suse.cz> References: <1460372892-8157-1-git-send-email-mhocko@kernel.org> <1460372892-8157-18-git-send-email-mhocko@kernel.org> <20160415130839.GJ32377@dhcp22.suse.cz> <20160416203135.GC15128@dhcp22.suse.cz> <20160422124730.GA11733@dhcp22.suse.cz> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-RedHat-Spam-Score: 0.18 (BAYES_50, DCC_REPUT_13_19, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_PASS) 74.125.82.65 mail-wm0-f65.google.com 74.125.82.65 mail-wm0-f65.google.com X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Scanned-By: MIMEDefang 2.78 on 10.5.110.31 X-loop: dm-devel@redhat.com X-Mailman-Approved-At: Wed, 27 Apr 2016 04:44:29 -0400 Cc: linux-mm@kvack.org, Andrew Morton , Shaohua Li , LKML , dm-devel@redhat.com Subject: Re: [dm-devel] [PATCH 17/19] dm: get rid of superfluous gfp flags X-BeenThere: dm-devel@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: device-mapper development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP [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 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 --- 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, ¶m_kernel, ioctl_flags, ¶m, ¶m_flags); + memalloc_noio_restore(noio_flag); if (r) return r;