Re: [RFC 4/4] NFS: Add forcekill mount option

[Joshua Watt <jpewhacker@gmail.com>]

On Fri, 2017-11-10 at 13:01 +1100, NeilBrown wrote:
> On Wed, Nov 08 2017, Joshua Watt wrote:
> 
> > Specifying the forcekill mount option causes umount() with the
> > MNT_FORCE
> > flag to not only kill all currently pending RPC tasks with -EIO,
> > but
> > also all future RPC tasks. This prevents the long delays caused by
> > tasks
> > queuing after rpc_killall_tasks() that can occur if the server
> > drops off
> > the network.
> > 
> > Signed-off-by: Joshua Watt <JPEWhacker@gmail.com>
> > ---
> >  fs/nfs/super.c                 | 17 ++++++++++++++---
> >  include/uapi/linux/nfs_mount.h |  1 +
> >  2 files changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> > index 66fda2dcadd0..d972f6289aca 100644
> > --- a/fs/nfs/super.c
> > +++ b/fs/nfs/super.c
> > @@ -90,6 +90,7 @@ enum {
> >  	Opt_resvport, Opt_noresvport,
> >  	Opt_fscache, Opt_nofscache,
> >  	Opt_migration, Opt_nomigration,
> > +	Opt_forcekill, Opt_noforcekill,
> >  
> >  	/* Mount options that take integer arguments */
> >  	Opt_port,
> > @@ -151,6 +152,8 @@ static const match_table_t
> > nfs_mount_option_tokens = {
> >  	{ Opt_nofscache, "nofsc" },
> >  	{ Opt_migration, "migration" },
> >  	{ Opt_nomigration, "nomigration" },
> > +	{ Opt_forcekill, "forcekill" },
> > +	{ Opt_noforcekill, "noforcekill" },
> >  
> >  	{ Opt_port, "port=%s" },
> >  	{ Opt_rsize, "rsize=%s" },
> > @@ -637,6 +640,7 @@ static void nfs_show_mount_options(struct
> > seq_file *m, struct nfs_server *nfss,
> >  		{ NFS_MOUNT_NORDIRPLUS, ",nordirplus", "" },
> >  		{ NFS_MOUNT_UNSHARED, ",nosharecache", "" },
> >  		{ NFS_MOUNT_NORESVPORT, ",noresvport", "" },
> > +		{ NFS_MOUNT_FORCEKILL, ",forcekill",
> > ",noforcekill" },
> >  		{ 0, NULL, NULL }
> >  	};
> >  	const struct proc_nfs_info *nfs_infop;
> > @@ -896,17 +900,18 @@ EXPORT_SYMBOL_GPL(nfs_show_stats);
> >   */
> >  void nfs_umount_begin(struct super_block *sb)
> >  {
> > -	struct nfs_server *server;
> > +	struct nfs_server *server = NFS_SB(sb);
> >  	struct rpc_clnt *rpc;
> > +	int kill_new_tasks = !!(server->flags &
> > NFS_MOUNT_FORCEKILL);
> >  
> >  	server = NFS_SB(sb);
> 
> Now that you initialized 'server' at declaration, you should really
> remove this (re)initialization.

Oops.

> 
> I'm not sure what I think of this patchset...
> You are setting a flag which causes "umount -f" to set a different
> flag.
> Why not just have "mount -o remount,XX" set the flag that you
> actually
> want to set.
> e.g
>    mount -o remount,serverfailed /mountpoint
> causes all rpcs to fail, and
>    mount -o remount,noserverfailed /mountpoint
> 
> causes all rpcs to be sent to the server.
> (or pick a different name than 'serverfailed').

Ah. I think I get it now :) Yes I will do that.

> 
> It isn't clear what the indirection buys us.
> 
> Thanks,
> NeilBrown
> 
> 
> 
> >  	/* -EIO all pending I/O */
> >  	rpc = server->client_acl;
> >  	if (!IS_ERR(rpc))
> > -		rpc_killall_tasks(rpc, 0);
> > +		rpc_killall_tasks(rpc, kill_new_tasks);
> >  	rpc = server->client;
> >  	if (!IS_ERR(rpc))
> > -		rpc_killall_tasks(rpc, 0);
> > +		rpc_killall_tasks(rpc, kill_new_tasks);
> >  }
> >  EXPORT_SYMBOL_GPL(nfs_umount_begin);
> >  
> > @@ -1334,6 +1339,12 @@ static int nfs_parse_mount_options(char
> > *raw,
> >  		case Opt_nomigration:
> >  			mnt->options &= ~NFS_OPTION_MIGRATION;
> >  			break;
> > +		case Opt_forcekill:
> > +			mnt->flags |= NFS_MOUNT_FORCEKILL;
> > +			break;
> > +		case Opt_noforcekill:
> > +			mnt->flags &= ~NFS_MOUNT_FORCEKILL;
> > +			break;
> >  
> >  		/*
> >  		 * options that take numeric values
> > diff --git a/include/uapi/linux/nfs_mount.h
> > b/include/uapi/linux/nfs_mount.h
> > index e44e00616ab5..66821542a38f 100644
> > --- a/include/uapi/linux/nfs_mount.h
> > +++ b/include/uapi/linux/nfs_mount.h
> > @@ -74,5 +74,6 @@ struct nfs_mount_data {
> >  
> >  #define NFS_MOUNT_LOCAL_FLOCK	0x100000
> >  #define NFS_MOUNT_LOCAL_FCNTL	0x200000
> > +#define NFS_MOUNT_FORCEKILL	0x400000
> >  
> >  #endif
> > -- 
> > 2.13.6