On Sun, 08 Nov 2009 18:56:07 +0100, Pavel Filipensky <Pavel.Filipensky at sun.com> wrote:
> On 11/08/09 14:07, Frank Batschulat (Home) wrote: >> On Sat, 07 Nov 2009 23:10:13 +0100, Pavel Filipensky <Pavel.Filipensky at >> sun.com> wrote: >> >>> can I get a code review for: 6897605 Deadlock in nfs4_active_reclaim() >>> on r_lock if file and its XATTR are hashed in the same r4hashq_t >>> >>> Webrev: >>> >>> http://cr.opensolaris.org/~pavelf/6897605 >>> >>> Background: >>> >>> If the system is low on memory, in can decide to reclaim memory from >>> "rnode4_cache". >>> This can lead to a deadlock if the rnode for a file uses extended >>> attributes and both >>> rnodes (file and its xattr) are hashed in the same r4hashq_t since the >>> nfsv4 code will ask for the same r_lock twice. >>> >>> The fix makes the reclaim less strict but safe - we avoid releasing of >>> xattr vnode. > >> I'm not much comfortable fix this fix. since this cached xattr dir vnode is >> accociated >> with the rnode we're going to deactivate - who's going to release the xattr >> vnode >> eventually ? >> > nfs4_active_data_reclaim() is not going to deactivate the 'main' rnode > which is pointing to the xattr dir vnode. > nfs4_active_data_reclaim() is only freeing various caches for the rnode, > the rnode itself remains active. > Since xattr dir vnode is one of the caches, we might decide not be so > thorough in freeing th ecaches > and keep this cache. > > If the 'main' rnode is deactivated sometimes in the future, r_xattr_dir > will be correctly released, > e.g. in r4inactive() or in rp4_addfree() - no leaking vnode. outch! yes of course, I somehow managed to ignore this minor but important detail. >> have you considered using VN_RELE_ASYNC(xattr vnode) here instead, which >> would >> issue a taskq thread handling the VN_RELE() for the asynchronously ? > > Thanks for this tip - I was not aware that there is an asynchronous > version of VN_RELE(). >> of course since we're in a low memory situation, a taskq may also have >> difficulties >> being created, but at least it will eventually and we have this all >> consistant. > > So we have 2 options: > > 1) remove VN_RELE(xattr) > pros: - simplify the code > cons: - less memory is freed > > 2) introduce VN_RELE_ASYNC(xattr) > pros: - more memory is reclaimed > cons: - slower code (extra work when the system is low on memory) > - adding a new code - need to create a new taskq (pool?) yepp, we'd have to create a new taskq as well, we probaly do not want to use the system taskq here. > Unless the ability to release as much memory as possible is critical, I > would prefer the simpler option - option #1. yepp, I'm happy with either way. 1) is clearly the less impact version. cheers frankB
