xfs
[Top] [All Lists]

Re: [PATCH 4/8] xfs: introduce xfs_rw_lock() helpers for locking the ino

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 4/8] xfs: introduce xfs_rw_lock() helpers for locking the inode
From: Alex Elder <aelder@xxxxxxx>
Date: Tue, 04 Jan 2011 19:54:41 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1294116518-14908-5-git-send-email-david@xxxxxxxxxxxxx>
References: <1294116518-14908-1-git-send-email-david@xxxxxxxxxxxxx> <1294116518-14908-5-git-send-email-david@xxxxxxxxxxxxx>
Reply-to: aelder@xxxxxxx
On Tue, 2011-01-04 at 15:48 +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> We need to obtain the i_mutex, i_iolock and i_ilock during the read
> and write paths. Add a set of wrapper functions to neatly
> encapsulate the lock ordering and shared/exclusive semantics to make
> the locking easier to follow and get right.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>

I like this change, but I think you missed a lock call.
I also notice there are some locking differences, and
I don't really question them but I wonder if you can
offer a little more explanation.

> ---
>  fs/xfs/linux-2.6/xfs_file.c |  123 
> ++++++++++++++++++++++++-------------------
>  1 files changed, 68 insertions(+), 55 deletions(-)
> 
> diff --git a/fs/xfs/linux-2.6/xfs_file.c b/fs/xfs/linux-2.6/xfs_file.c
> index 33a688c..0d6111e 100644
> --- a/fs/xfs/linux-2.6/xfs_file.c
> +++ b/fs/xfs/linux-2.6/xfs_file.c

. . .

> @@ -262,22 +296,21 @@ xfs_file_aio_read(
>       if (XFS_FORCED_SHUTDOWN(mp))
>               return -EIO;
>  
> -     if (unlikely(ioflags & IO_ISDIRECT))
> -             mutex_lock(&inode->i_mutex);
> -     xfs_ilock(ip, XFS_IOLOCK_SHARED);
> -
>       if (unlikely(ioflags & IO_ISDIRECT)) {
> +             xfs_rw_ilock(ip, XFS_IOLOCK_EXCL);
> +

Previously only XFS_IOLOCK_SHARED was used here.
I understand that using the IOLOCK_EXCL now gets
the desired mutex_lock() call.  Is the previous
code in error here though?  Can you anticipate
any different behavior because of this lock change?
Does this specific change justify separating it
into a small patch just before this one?

>               if (inode->i_mapping->nrpages) {
>                       ret = -xfs_flushinval_pages(ip,
>                                       (iocb->ki_pos & PAGE_CACHE_MASK),
>                                       -1, FI_REMAPF_LOCKED);
> +                     if (ret) {
> +                             xfs_rw_iunlock(ip, XFS_IOLOCK_EXCL);
> +                             return ret;
> +                     }
>               }
> -             mutex_unlock(&inode->i_mutex);
> -             if (ret) {
> -                     xfs_iunlock(ip, XFS_IOLOCK_SHARED);
> -                     return ret;
> -             }
> -     }
> +             xfs_rw_ilock_demote(ip, XFS_IOLOCK_EXCL);
> +     } else
> +             xfs_rw_ilock(ip, XFS_IOLOCK_SHARED);
>  
>       trace_xfs_file_read(ip, size, iocb->ki_pos, ioflags);
>  

. . .

> @@ -386,14 +419,13 @@ xfs_file_splice_write(
>       if (XFS_FORCED_SHUTDOWN(ip->i_mount))
>               return -EIO;
>  
> -     xfs_ilock(ip, XFS_IOLOCK_EXCL);
> +     xfs_rw_ilock(ip, XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL);

Similar sentiments here.  We will now be acquiring i_mutex
here where previously we did not.  Is that OK?

>       new_size = *ppos + count;
>  
> -     xfs_ilock(ip, XFS_ILOCK_EXCL);
>       if (new_size > ip->i_size)
>               ip->i_new_size = new_size;
> -     xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +     xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
>  
>       trace_xfs_file_splice_write(ip, count, *ppos, ioflags);
>  

. . .

> @@ -631,21 +662,16 @@ xfs_file_aio_write(
>  relock:
>       if (ioflags & IO_ISDIRECT) {
>               iolock = XFS_IOLOCK_SHARED;
> -             need_i_mutex = 0;
>       } else {
>               iolock = XFS_IOLOCK_EXCL;
> -             need_i_mutex = 1;
> -             mutex_lock(&inode->i_mutex);
>       }
>  
> -     xfs_ilock(ip, XFS_ILOCK_EXCL|iolock);
> -

Maybe I'm missing something, but I think you want to
insert this here:
        xfs_rw_ilock(ip, XFS_ILOCK_EXCL|iolock);
...because (for starters) if generic_write_checks()
returns an error below you're going to be calling
the unlock routine.

>  start:
>       ret = generic_write_checks(file, &pos, &count,
>                                       S_ISBLK(inode->i_mode));
>       if (ret) {
> -             xfs_iunlock(ip, XFS_ILOCK_EXCL|iolock);
> -             goto out_unlock_mutex;
> +             xfs_rw_iunlock(ip, XFS_ILOCK_EXCL|iolock);
> +             return ret;
>       }
>  
>       if (ioflags & IO_ISDIRECT) {
> @@ -654,16 +680,14 @@ start:
>                               mp->m_rtdev_targp : mp->m_ddev_targp;
>  
>               if ((pos & target->bt_smask) || (count & target->bt_smask)) {
> -                     xfs_iunlock(ip, XFS_ILOCK_EXCL|iolock);
> +                     xfs_rw_iunlock(ip, XFS_ILOCK_EXCL|iolock);
>                       return XFS_ERROR(-EINVAL);
>               }
>  

One can get a little lost in this code.  I don't know if
this comment is exactly right, but something like it might
be helpful (while you're in here).

                /*
                 * For direct I/O, if there are cached pages or
                 * we're extending the file, we need IOLOCK_EXCL
                 * until we're sure the bytes at the new EOF have
                 * been zeroed and/or the cached pages are flushed
                 * out.  Upgrade the I/O lock and start again.
                 */

> -             if (!need_i_mutex && (mapping->nrpages || pos > ip->i_size)) {
> -                     xfs_iunlock(ip, XFS_ILOCK_EXCL|iolock);
> +             if (iolock != XFS_IOLOCK_EXCL &&
> +                 (mapping->nrpages || pos > ip->i_size)) {
> +                     xfs_rw_iunlock(ip, XFS_ILOCK_EXCL|iolock);
>                       iolock = XFS_IOLOCK_EXCL;
> -                     need_i_mutex = 1;
> -                     mutex_lock(&inode->i_mutex);
> -                     xfs_ilock(ip, XFS_ILOCK_EXCL|iolock);
>                       goto start;
>               }
>       }

. . .




<Prev in Thread] Current Thread [Next in Thread>