[OpenAFS-devel] OpenAFS, Linux and truncate_inode_pages()

[Rainer Toebbicke]

chas williams - CONTRACTOR wrote:
> In message <4405CB2C.1010702@pclella.cern.ch>,Rainer Toebbicke writes:
> 
>>Corrupted on the local client, in the chunk file (witnessed by grep on 
>>the cache files).  File normal again after 'fs flush'.
> 
> 
> i guess this wouldnt make me to suspect something wrong in osi_vm.c.

I wouldn't either now but the problem was larger initially.

> regardless, the following might be a little more "correct".  give
> it a try.
> 
> Index: src/afs/LINUX/osi_vm.c
> ===================================================================
> RCS file: /cvs/openafs/src/afs/LINUX/osi_vm.c,v
> retrieving revision 1.16.2.1
> diff -u -u -r1.16.2.1 osi_vm.c
> --- src/afs/LINUX/osi_vm.c	11 Jul 2005 19:29:56 -0000	1.16.2.1
> +++ src/afs/LINUX/osi_vm.c	1 Mar 2006 20:50:45 -0000
> @@ -51,6 +51,8 @@
>      if (avc->opens != 0)
>  	return EBUSY;
>  
> +    down(&ip->i_sem);
> +

I'd prefer a
	osi_Assert(down_trylock(&ip->i_sem) == 0);
here. At the expense of a handful of cycles it clearly states that the 
AFS locking logic should prevail.


>  #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,0)
>      truncate_inode_pages(&ip->i_data, 0);
>  #elif LINUX_VERSION_CODE >= KERNEL_VERSION(2,2,15)
> @@ -58,6 +60,9 @@
>  #else
>      invalidate_inode_pages(ip);
>  #endif
> +
> +    up(&ip->i_sem);
> +
>      return 0;
>  }
>  
> @@ -100,18 +105,28 @@
>  void
>  osi_VM_StoreAllSegments(struct vcache *avc)
>  {
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,5)
>      struct inode *ip = AFSTOV(avc);
> +    int err;
> +    int need_unlock = 1;
>  
> -#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,5)
> -    /* filemap_fdatasync() only exported in 2.4.5 and above */
>      ReleaseWriteLock(&avc->lock);
>      AFS_GUNLOCK();
> -#if defined(AFS_LINUX26_ENV)
> -    filemap_fdatawrite(ip->i_mapping);
> +
> +    if (down_trylock(&ip->i_sem))
> +	need_unlock = 0;
> +
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> +    err = filemap_fdatawrite(ip->i_mapping);


This doesn't look sane to me! If you manage to down the i_sem then 
you're ok if the i_sem is really needed for filemap_fdatasync, however 
if you don't then you don't really know whether it's because you 
already did it yourself higher up or if "somebody else" (not 
necessarily AFS code) did and is then dangerously close to messing 
around with the structures.

>  #else
> -    filemap_fdatasync(ip->i_mapping);
> +    err = filemap_fdatasync(ip->i_mapping);
>  #endif
> -    filemap_fdatawait(ip->i_mapping);
> +    if (err == 0)
> +	filemap_fdatawait(ip->i_mapping);
> +
> +    if (need_unlock)
> +	up(&ip->i_sem);
> +
>      AFS_GLOCK();
>      ObtainWriteLock(&avc->lock, 121);
>  #endif
> @@ -124,17 +139,20 @@
>  void
>  osi_VM_FlushPages(struct vcache *avc, struct AFS_UCRED *credp)
>  {
> -#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,0)
>      struct inode *ip = AFSTOV(avc);
>  
> +    down(&ip->i_sem);

Again:
	osi_Assert(down_trylock(&ip->i_sem) == 0);

> +
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,0)
>      truncate_inode_pages(&ip->i_data, 0);
>  #elif LINUX_VERSION_CODE >= KERNEL_VERSION(2,2,15)
> -    struct inode *ip = AFSTOV(avc);
>  
>      truncate_inode_pages(ip, 0);
>  #else
> -    invalidate_inode_pages(AFSTOV(avc));
> +    invalidate_inode_pages(ip);
>  #endif
> +
> +    up(&ip->i_sem);
>  }
>  
>  /* Purge pages beyond end-of-file, when truncating a file.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
Rainer Toebbicke
European Laboratory for Particle Physics(CERN) - Geneva, Switzerland
Phone: +41 22 767 8985       Fax: +41 22 767 7155