Re: [PATCH 6/7] nonblocking aio: xfs

[Christoph Hellwig <hch@infradead.org>]

On Mon, Feb 13, 2017 at 08:46:02PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Please Cc the while series to all lists, otherwiwe it's impossible
to review the thing.

> 
> If IOCB_NONBLOCKING is set:
> 	+ Check if writing beyond end of file, if yes return EAGAIN
> 	- check if writing to a hole which does not have allocated
> 	  file blocks.
> 	- Check if i_rwsem is immediately lockable in xfs_rw_ilock()

Why the + vs - above? 

> -static inline void
> +static inline int
>  xfs_rw_ilock(

This function has been removed a while ago.

>  
> +	if (iocb->ki_flags & IOCB_NONBLOCKING) {
> +		struct xfs_bmbt_irec	imap;
> +		xfs_fileoff_t           offset_fsb, end_fsb;
> +		int			nimaps = 1, ret = 0;
> +		end_fsb = XFS_B_TO_FSB(ip->i_mount, iocb->ki_pos + count);
> +		if (XFS_B_TO_FSB(ip->i_mount, i_size_read(inode)) < end_fsb)
> +			return -EAGAIN;

Bogus check, XFS supports async write beyond EOF if the space is
preallocated.

> +		/* Check if it is an unallocated hole */
> +		offset_fsb = XFS_B_TO_FSBT(ip->i_mount, iocb->ki_pos);
> +
> +		ret = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, &imap,
> +				&nimaps, 0);
> +		if (ret)
> +			return ret;
> +		if (!nimaps || imap.br_startblock == HOLESTARTBLOCK)
> +			return -EAGAIN;

This would need the ilock.  But extent list lookups are expensive,
so please don't add another one here.  Just return when we hit the
first unallocated extent in xfs_bmapi_write - either a short write or
-EAGAIN if nothing has been written.

>  	error = xfs_break_layouts(inode, iolock, true);
>  	if (error)
>  		return error;

Additionally this can drop the iolock, so you might get a new
hole after it.