xfs
[Top] [All Lists]

Re: [PATCH 4/5] xfs: push the ilock into xfs_zero_eof

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 4/5] xfs: push the ilock into xfs_zero_eof
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 27 Mar 2012 12:15:40 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120326211603.654869525@xxxxxxxxxxxxxxxxxxxxxx>
References: <20120326211421.518374058@xxxxxxxxxxxxxxxxxxxxxx> <20120326211603.654869525@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Mar 26, 2012 at 05:14:25PM -0400, Christoph Hellwig wrote:
> Instead of calling xfs_zero_eof with the ilock held only take it internally
> for the minimall required critical section around xfs_bmapi_read.  This
> also requires changing the calling convention for xfs_zero_last_block
> slightly.  The actual zeroing operation is still serialized by the iolock,
> which must be taken exclusively over the call to xfs_zero_eof.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
.....
> +     struct xfs_mount        *mp = ip->i_mount;
> +     xfs_fileoff_t           last_fsb = XFS_B_TO_FSBT(mp, isize);
> +     int                     zero_offset = XFS_B_FSB_OFFSET(mp, isize);
> +     int                     zero_len;
> +     int                     nimaps = 1;
> +     int                     error = 0;
> +     struct xfs_bmbt_irec    imap;
>  
> -     last_fsb = XFS_B_TO_FSBT(mp, isize);
> -     nimaps = 1;
> +     xfs_ilock(ip, XFS_ILOCK_EXCL);
>       error = xfs_bmapi_read(ip, last_fsb, 1, &imap, &nimaps, 0);
> +     xfs_iunlock(ip, XFS_ILOCK_EXCL);

Does that even need to be an exclusive lock? a shared lock is all
that is needed to do a lookup, and this is just a lookup...

....

> @@ -521,23 +503,18 @@ xfs_zero_eof(
>       while (start_zero_fsb <= end_zero_fsb) {
>               nimaps = 1;
>               zero_count_fsb = end_zero_fsb - start_zero_fsb + 1;
> +
> +             xfs_ilock(ip, XFS_ILOCK_EXCL);
>               error = xfs_bmapi_read(ip, start_zero_fsb, zero_count_fsb,
>                                         &imap, &nimaps, 0);
> -             if (error) {
> -                     ASSERT(xfs_isilocked(ip, 
> XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL));
> +             xfs_iunlock(ip, XFS_ILOCK_EXCL);

Same question - it is a read lookup so why do we need exclusive
locking here?

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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