On 02/26/2013 08:01 AM, Gregory Farnum wrote:
> On Fri, Feb 22, 2013 at 8:31 PM, Yan, Zheng <zheng.z.yan@xxxxxxxxx> wrote:
>> On 02/23/2013 02:54 AM, Gregory Farnum wrote:
>>> I haven't spent that much time in the kernel client, but this patch
>>> isn't working out for me. In particular, I'm pretty sure we need to
>>> preserve this:
>>>
>>>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
>>>> index 5d5c32b..b9d8417 100644
>>>> --- a/fs/ceph/caps.c
>>>> +++ b/fs/ceph/caps.c
>>>> @@ -2067,12 +2067,6 @@ static int try_get_cap_refs(struct ceph_inode_info *ci, int need, int want,
>>>> }
>>>> have = __ceph_caps_issued(ci, &implemented);
>>>>
>>>> - /*
>>>> - * disallow writes while a truncate is pending
>>>> - */
>>>> - if (ci->i_truncate_pending)
>>>> - have &= ~CEPH_CAP_FILE_WR;
>>>> -
>>>> if ((have & need) == need) {
>>>> /*
>>>> * Look at (implemented & ~have & not) so that we keep waiting
>>>
>>> Because if there's a pending truncate, we really can't write. You do
>>> handle it in the particular case of doing buffered file writes, but
>>> these caps are a marker of permission, and the client shouldn't have
>>> write permission to a file until it's up to date on the truncates. Or
>>> am I misunderstanding something?
>>
>> pending vmtruncate is only relevant to buffered write case. If client doesn't have 'b' cap,
>> the page cache is empty, and __ceph_do_pending_vmtruncate is no-op. For buffered write,
>> this patch only affects situation that clients receives truncate request from MDS in the
>> middle of write.
> It looks to me like truncates can get queued for later, so that's not the case?
> And how could the client receive a truncate while in the middle of
> writing? Either it's got the write caps (in which case nobody else can
> truncate), or it shouldn't be writing...I suppose if somebody enables
> lazyIO that would do it, but again in that case I imagine we'd rather
> do the truncate before writing:
Commit 22cddde104 make buffered write get/put caps for individual page. So the MDS can
revoke the write caps in the middle of write.
>
>> I think it's better to do vmtruncate after write finishes.
> What if you're writing past the truncate point? Then your written data
> would be truncated off even though it should have been a file
> extension.
>
I was wrong, I thought the order of write and truncate is not important. Now I'm worry about
the correctness of commit 22cddde104, we probably should revert that commit.
>
>>>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>>>> index a1e5b81..bf7849a 100644
>>>> --- a/fs/ceph/file.c
>>>> +++ b/fs/ceph/file.c
>>>> @@ -653,7 +653,6 @@ static ssize_t ceph_aio_read(struct kiocb *iocb, const struct iovec *iov,
>>>> dout("aio_read %p %llx.%llx %llu~%u trying to get caps on %p\n",
>>>> inode, ceph_vinop(inode), pos, (unsigned)len, inode);
>>>> again:
>>>> - __ceph_do_pending_vmtruncate(inode);
>>>
>>> There doesn't seem to be any kind of replacement for this? We need to
>>> do any pending truncates before reading or we might get stale data
>>> back.
>>
>> generic_file_aio_read checks i_size when coping data to user buffer, so the user program can't
>> get stale data. This __ceph_do_pending_vmtruncate is not protected by i_mutex, it's a potential
>> bug, that's the reason I remove it.
>
> Oh, you're right about the locking (I think, anyway?). However, i_size
> doesn't protect us — we might for instance have truncated to zero and
> then back up to the original file size. :( But that doesn't look like
> it's handled correctly anyway (racy) — am I misreading that badly or
> is the infrastructure in fact broken in that case?
You are right. probably we can fix the race by disallowing both read and write when
there is pending truncate.
Thank you
Yan, Zheng
>
> As I said, I don't spend much time in the kernel client, but I looked
> around a bit more and this is what I came up with. Education, if
> nothing else. ;)
> -Greg
>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
