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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 02/14/2017 01:43 AM, Christoph Hellwig wrote:
> On Mon, Feb 13, 2017 at 08:46:02PM -0600, Goldwyn Rodrigues wrote:
>> From: Goldwyn Rodrigues <rgoldwyn@xxxxxxxx>
> 
> 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? 

A mistake. It does not mean anything besides bullets.

> 
>> -static inline void
>> +static inline int
>>  xfs_rw_ilock(
> 
> This function has been removed a while ago.

And thanks for putting in xfs_ilock_nowait(). This can't be done in a
cleaner way. I was very skeptical of adding yet another flag.

> 
>>  
>> +	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.

I assume this will be covered by xfs_bmapi_write().

> 
>> +		/* 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.

So in xfs_bmapi_write (and referring to), I would need a new flag, say
XFS_BMAPI_NOALLOC which would bail to error0, setting error=-EAGAIN if
need_alloc or was_delay is set. This flag XFS_BMAPI_NOALLOC is set in
xfs_iomap_write_direct(). I did not understand short writes. Where can I
get a short write?

If I understand correctly, we do add the flag.

> 
>>  	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.
> 

-- 
Goldwyn
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux