[Ocfs2-devel] [PATCH 1/1] Ocfs2: Teach 'coherency=full' O_DIRECT writes to correctly up_read i_alloc_sem.

[Tao Ma <tao.ma@oracle.com>]

Hi Tristan,

On 11/29/2010 05:21 PM, Tristan Ye wrote:
> Due to newly-introduced 'coherency=full' O_DIRECT writes also takes the EX
> rw_lock like buffered writes did(rw_level == 1), it turns out messing the
> usage of 'level' in ocfs2_dio_end_io() up, which caused i_alloc_sem being
> failed to get up_read'd correctly.
>
> This patch tries to teach ocfs2_dio_end_io to understand well on all locking
> stuffs by explicitly introducing a new bit for i_alloc_sem in iocb's private
> data, just like what we did for rw_lock.
>
> Signed-off-by: Tristan Ye<tristan.ye@oracle.com>
> ---
>   fs/ocfs2/aops.c |    6 ++++--
>   fs/ocfs2/aops.h |    6 ++++++
>   fs/ocfs2/file.c |    9 +++++++--
>   3 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index f1e962c..857e013 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -568,7 +568,7 @@ static void ocfs2_dio_end_io(struct kiocb *iocb,
>   			     bool is_async)
>   {
>   	struct inode *inode = iocb->ki_filp->f_path.dentry->d_inode;
> -	int level;
> +	int level, sem_locked;
Is sem_locked really needed here? At least from your code below, we 
don't need it if we can change the sequence somehow.
>
>   	/* this io's submitter should not have unlocked this before we could */
>   	BUG_ON(!ocfs2_iocb_is_rw_locked(iocb));
> @@ -576,7 +576,9 @@ static void ocfs2_dio_end_io(struct kiocb *iocb,
>   	ocfs2_iocb_clear_rw_locked(iocb);
>
>   	level = ocfs2_iocb_rw_locked_level(iocb);
> -	if (!level)
> +	sem_locked = ocfs2_iocb_is_sem_locked(iocb);
> +	ocfs2_iocb_clear_sem_locked(iocb);
> +	if (sem_locked)
>   		up_read(&inode->i_alloc_sem);
>   	ocfs2_rw_unlock(inode, level);
>
> diff --git a/fs/ocfs2/aops.h b/fs/ocfs2/aops.h
> index 76bfdfd..c7a3e5f 100644
> --- a/fs/ocfs2/aops.h
> +++ b/fs/ocfs2/aops.h
> @@ -72,4 +72,10 @@ static inline void ocfs2_iocb_set_rw_locked(struct kiocb *iocb, int level)
>   	clear_bit(0, (unsigned long *)&iocb->private)
>   #define ocfs2_iocb_rw_locked_level(iocb) \
>   	test_bit(1, (unsigned long *)&iocb->private)
> +#define ocfs2_iocb_set_sem_locked(iocb) \
> +	set_bit(2, (unsigned long *)&iocb->private)
> +#define ocfs2_iocb_clear_sem_locked(iocb) \
> +	clear_bit(2, (unsigned long *)&iocb->private)
> +#define ocfs2_iocb_is_sem_locked(iocb) \
> +	test_bit(2, (unsigned long *)&iocb->private)
>   #endif /* OCFS2_FILE_H */
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index 77b4c04..0e9d729 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -2246,7 +2246,10 @@ relock:
>   	if (direct_io) {
>   		down_read(&inode->i_alloc_sem);
>   		have_alloc_sem = 1;
> -	}
> +		/* communicate with ocfs2_dio_end_io */
> +		ocfs2_iocb_set_sem_locked(iocb);
> +	} else
> +		ocfs2_iocb_clear_sem_locked(iocb);
>
>   	/*
>   	 * Concurrent O_DIRECT writes are allowed with
Sorry, but why you clear the sem lock here? It doesn't make sense if you 
read the code for the first time since we have't set it before. So it 
looks a little bit strange.

I guess maybe we can clear it when we do up_read(&inode->i_alloc_sem)?

Or another way, why not put it with the set of rw_level.
	/* communicate with ocfs2_dio_end_io */
         ocfs2_iocb_set_rw_locked(iocb, rw_level);
+	ocfs2_iocb_set_sem_locked(iocb, have_alloc_sem);

Regards,
Tao