Re: [PATCH 4/4] NFS/SUNRPC: Remove other deadlock-avoidance mechanisms in nfs_release_page()

[NeilBrown]

On Tue, 16 Sep 2014 21:32:43 -0400 Trond Myklebust
<trond.myklebust@xxxxxxxxxxxxxxx> wrote:

> On Tue, Sep 16, 2014 at 9:10 PM, NeilBrown <neilb@xxxxxxx> 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));

Attachment: signature.asc
Description: PGP signature