From patchwork Wed Sep 17 03:12:18 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: NeilBrown X-Patchwork-Id: 4922481 Return-Path: X-Original-To: patchwork-linux-nfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id B1D399F32F for ; Wed, 17 Sep 2014 03:10:53 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 4AA4520142 for ; Wed, 17 Sep 2014 03:12:34 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 36B0A20103 for ; Wed, 17 Sep 2014 03:12:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753676AbaIQDMb (ORCPT ); Tue, 16 Sep 2014 23:12:31 -0400 Received: from cantor2.suse.de ([195.135.220.15]:50102 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753320AbaIQDMb (ORCPT ); Tue, 16 Sep 2014 23:12:31 -0400 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 03935AD3D; Wed, 17 Sep 2014 03:12:29 +0000 (UTC) Date: Wed, 17 Sep 2014 13:12:18 +1000 From: NeilBrown To: Trond Myklebust Cc: Peter Zijlstra , Andrew Morton , Ingo Molnar , Devel FS Linux , linux-mm@kvack.org, Linux NFS Mailing List , Linux Kernel mailing list , Jeff Layton Subject: Re: [PATCH 4/4] NFS/SUNRPC: Remove other deadlock-avoidance mechanisms in nfs_release_page() Message-ID: <20140917131218.4f664002@notabene.brown> In-Reply-To: References: <20140916051911.22257.24658.stgit@notabene.brown> <20140916053135.22257.46476.stgit@notabene.brown> <20140917111017.499eb3a9@notabene.brown> X-Mailer: Claws Mail 3.10.1-123-gae895c (GTK+ 2.24.22; x86_64-suse-linux-gnu) MIME-Version: 1.0 Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Spam-Status: No, score=-7.5 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_TVD_MIME_EPI, 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 On Tue, 16 Sep 2014 21:32:43 -0400 Trond Myklebust wrote: > On Tue, Sep 16, 2014 at 9:10 PM, NeilBrown wrote: > > > > However ... something else occurs to me. We could use the bdi congestion > > markers to guide the timeout. > > When the wait for PG_private times out, or when a connection re-establishment > > is required (and maybe other similar times) we could set_bdi_congested(). > > Then in nfs_release_page() we could completely avoid the wait if > > bdi_write_congested(). > > > > The congestion setting should encourage vmscan away from the filesystem so it > > won't keep calling nfs_release_page() which is a bonus. > > > > Setting bdi_congestion from the RPC layer might be awkward from a layering > > perspective, but probably isn't necessary. > > > > Would the following allay your concerns? The change to > > nfs_inode_remove_request ensures that any congestion is removed when a > > 'commit' completes. > > > > We certainly could keep the PF_FSTRANS setting in the SUNRPC layer - that was > > why it was a separate patch. It would be nice to find a uniform solution > > though. > > > > Thanks, > > NeilBrown > > > > > > > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c > > index 5949ca37cd18..bc674ad250ce 100644 > > --- a/fs/nfs/file.c > > +++ b/fs/nfs/file.c > > @@ -477,10 +477,15 @@ static int nfs_release_page(struct page *page, gfp_t gfp) > > * benefit that someone else can worry about the freezer. > > */ > > if (mapping) { > > + struct nfs_server *nfss = NFS_SERVER(mapping->host); > > nfs_commit_inode(mapping->host, 0); > > - if ((gfp & __GFP_WAIT)) > > + if ((gfp & __GFP_WAIT) && > > + !bdi_write_congested(&nfss->backing_dev_info)) > > wait_on_page_bit_killable_timeout(page, PG_private, > > HZ); > > + if (PagePrivate(page)) > > + set_bdi_congested(&nfss->backing_dev_info, > > + BLK_RW_ASYNC); > > } > > /* If PagePrivate() is set, then the page is not freeable */ > > if (PagePrivate(page)) > > diff --git a/fs/nfs/write.c b/fs/nfs/write.c > > index 700e7a865e6d..3ab122e92c9d 100644 > > --- a/fs/nfs/write.c > > +++ b/fs/nfs/write.c > > @@ -726,6 +726,7 @@ static void nfs_inode_remove_request(struct nfs_page *req) > > struct inode *inode = req->wb_context->dentry->d_inode; > > struct nfs_inode *nfsi = NFS_I(inode); > > struct nfs_page *head; > > + struct nfs_server *nfss = NFS_SERVER(inode); > > > > if (nfs_page_group_sync_on_bit(req, PG_REMOVE)) { > > head = req->wb_head; > > @@ -742,6 +743,9 @@ static void nfs_inode_remove_request(struct nfs_page *req) > > spin_unlock(&inode->i_lock); > > } > > > > + if (atomic_long_read(&nfss->writeback) < NFS_CONGESTION_OFF_THRESH) > > + clear_bdi_congested(&nfss->backing_dev_info, BLK_RW_ASYNC); > > Hmm.... We already have this equivalent functionality in > nfs_end_page_writeback(), so adding it to nfs_inode_remove_request() > is just causing duplication as far as the stable writeback path is > concerned. How about adding it to nfs_commit_release_pages() instead? > > Otherwise, yes, the above does indeed look at if it has merit. Have > you got a good test? > Altered patch below. I'll post a proper one after some testing. For testing I create a memory pressure load with: ============================= #!/bin/bash umount /mnt/ramdisk umount /mnt/ramdisk mount -t tmpfs -o size=4G none /mnt/ramdisk #swapoff -a i=0 while [ $i -le 10000 ]; do i=$(($i+1)) dd if=/dev/zero of=/mnt/ramdisk/testdata.dd bs=1M count=6500 date done ============================== Where the '4G' matches memory size, and then write out to an NFS file with ================================= #!/bin/bash umount /mnt2 /mnt3 umount /mnt2 /mnt3 mount /dev/sdd /mnt2 exportfs -avu exportfs -av mount $* 127.0.0.1:/mnt2 /mnt3 for j in {1..100}; do i=1 while [ $i -le 10000 ]; do echo "Step $i" date +%H:%M:%S i=$(($i+1)) zcat /boot/vmlinux-3.13.3-1-desktop.gz | uuencode - date +%H:%M:%S done | dd of=/mnt3/testdat.file bs=1M done ==================================== Pick your own way to create a large file of random data .. though probably /dev/zero would do. With both those going for a few hours the current kernel will deadlock. With my patches it doesn't. I'll see if I can come up with some way to measure maximum delay in try_to_free_pages() and see how the 'congestion' change affects that. Thanks, NeilBrown ---------------------- diff --git a/fs/nfs/file.c b/fs/nfs/file.c index 5949ca37cd18..bc674ad250ce 100644 --- a/fs/nfs/file.c +++ b/fs/nfs/file.c @@ -477,10 +477,15 @@ static int nfs_release_page(struct page *page, gfp_t gfp) * benefit that someone else can worry about the freezer. */ if (mapping) { + struct nfs_server *nfss = NFS_SERVER(mapping->host); nfs_commit_inode(mapping->host, 0); - if ((gfp & __GFP_WAIT)) + if ((gfp & __GFP_WAIT) && + !bdi_write_congested(&nfss->backing_dev_info)) wait_on_page_bit_killable_timeout(page, PG_private, HZ); + if (PagePrivate(page)) + set_bdi_congested(&nfss->backing_dev_info, + BLK_RW_ASYNC); } /* If PagePrivate() is set, then the page is not freeable */ if (PagePrivate(page)) diff --git a/fs/nfs/write.c b/fs/nfs/write.c index 700e7a865e6d..8d4aae9d977a 100644 --- a/fs/nfs/write.c +++ b/fs/nfs/write.c @@ -1641,6 +1641,7 @@ static void nfs_commit_release_pages(struct nfs_commit_data *data) struct nfs_page *req; int status = data->task.tk_status; struct nfs_commit_info cinfo; + struct nfs_server *nfss; while (!list_empty(&data->pages)) { req = nfs_list_entry(data->pages.next); @@ -1674,6 +1675,10 @@ static void nfs_commit_release_pages(struct nfs_commit_data *data) next: nfs_unlock_and_release_request(req); } + nfss = NFS_SERVER(data->inode); + if (atomic_long_read(&nfss->writeback) < NFS_CONGESTION_OFF_THRESH) + clear_bdi_congested(&nfss->backing_dev_info, BLK_RW_ASYNC); + nfs_init_cinfo(&cinfo, data->inode, data->dreq); if (atomic_dec_and_test(&cinfo.mds->rpcs_out)) nfs_commit_clear_lock(NFS_I(data->inode));