On 02/15/2017 09:30 AM, Goldwyn Rodrigues wrote:
>
>
> 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.
Replying to myself to correct myself.
On reading a bit more, I figured that we perform
xfs_file_iomap_begin->xfs_iomap_write_direct. At this point we have
already performed xfs_bmapi_read(). So, a check in xfs_file_iomap_begin
should be good enough. So, the flag required would be with iomap flags,
say IOMAP_NONBLOCKING. IOW, we don't need to go all the way to
xfs_bmap_write() and return when imap.br_startblock == HOLESTARTBLOCK.
>
>>
>>> 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
