From owner-freebsd-fs@freebsd.org Tue Dec 20 07:15:19 2016 Return-Path: Delivered-To: freebsd-fs@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B2EA5C89231 for ; Tue, 20 Dec 2016 07:15:19 +0000 (UTC) (envelope-from truckman@FreeBSD.org) Received: from gw.catspoiler.org (unknown [IPv6:2602:304:b010:ef20::f2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "gw.catspoiler.org", Issuer "gw.catspoiler.org" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 7BE2E1438 for ; Tue, 20 Dec 2016 07:15:19 +0000 (UTC) (envelope-from truckman@FreeBSD.org) Received: from FreeBSD.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.15.2/8.15.2) with ESMTP id uBK7FAlL002072; Mon, 19 Dec 2016 23:15:14 -0800 (PST) (envelope-from truckman@FreeBSD.org) Message-Id: <201612200715.uBK7FAlL002072@gw.catspoiler.org> Date: Mon, 19 Dec 2016 23:15:10 -0800 (PST) From: Don Lewis Subject: Re: ESTALE after cwd deleted by same NFS client To: rmacklem@uoguelph.ca cc: cperciva@tarsnap.com, freebsd-fs@freebsd.org In-Reply-To: MIME-Version: 1.0 Content-Type: TEXT/plain; charset=us-ascii X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Dec 2016 07:15:19 -0000 On 19 Dec, Rick Macklem wrote: > Colin Percival wrote: > >>On 12/16/16 12:14, Colin Percival wrote: >>> making this change in nfs_lookup >>>> --- sys/fs/nfsclient/nfs_clvnops.c (revision 310132) >>>> +++ sys/fs/nfsclient/nfs_clvnops.c (working copy) >>>> @@ -1144,7 +1144,7 @@ >>>> *vpp = NULLVP; >>>> } >>>> >>>> - if (error != ENOENT) { >>>> + if (error != ENOENT && error != ESTALE) { >>>> if (NFS_ISV4(dvp)) >>>> error = nfscl_maperr(td, error, (uid_t)0, >>>> (gid_t)0); >>> >>> fixes the case I described above (for some definition of "fixes" -- I'm not >>> sure if this is the correct way of handling ESTALE here?) but I'm still seeing >>> ESTALEs from buildworld's cleandir so I think there must be some other place >>> where something odd is happening. >> >>Further information: In addition to the "lookup relative to a directory which >>has been deleted out from underneath us" case which causes ESTALE to land in >>nfs_lookup, the cleandir step of buildworld results in ESTALE being returned >>by nfsrpc_getattr into nfs_getattr (landing ultimately in getcwd), and ESTALE >>being returned by nfsrpc_accessrpc into nfs34_access_otw (landing ultimately >>in stat and lstat). >> >>In UFS there are checks for effnlink == 0 which result in e.g. ufs_lookup >>returning ENOENT; would it make sense to add NREMOVED to struct nfsnode.n_flag >>and check this in the appropriate nfs_* calls? > To be honest, I can't think of a reason why userland would ever want to see ESTALE? > The function you see above "nfscl_maperr()" could easily map all ESTALEs to ENOENTs? > - The question is: "Would returning ENOENT for stat(2) and access(2) actually make the > buildworld happy? > if Yes > - then mapping ESTALE->ENOENT makes sense for most/all VOP_xxx() calls. > else > - I don't see any point in returning a different error and there might be some > code out there somewhere that depends on seeing ESTALE (I doubt it, but???). > > The real problem here is that the directory has a reference count on it when it is > rmdir'd. POSIX file systems keep the data until the reference count goes to 0, but > NFS isn't POSIX. > --> The cheat for regular files is "sillyrename". This could be done for directories, > but there are multiple comments in the code (not put there by me) that say > "no sillyrename for directories". > #1 Does this imply something breaks when you do sillyrename for dirs? > OR > #2 Does it mean no one has bothered to implement it? > Since implementing it would have been pretty easy, I have to suspect #1, which > means I would be reluctant to do it, at least by default. > --> Maybe I'll send you a patch that does sillyrename for dirs which you can test. > If it fixes buildworld, then it could be considered for head as a non-default option? Sillyrename makes sense for files because you can continue to read and write to a file after it is unlinked, until it is closed and it totally vanishes. It doesn't make sense for directories since you can't rmdir() a directory until it is empty. Once it is rmdir()ed, then you can't create any new entries in the directory because that would mean that you are creating a disconnnected subtree in the filesystem.