From: Ritesh Harjani <riteshh@linux.ibm.com>
To: Jan Kara <jack@suse.cz>
Cc: tytso@mit.edu, linux-ext4@vger.kernel.org,
linux-fsdevel@vger.kernel.org, mbobrowski@mbobrowski.org,
joseph.qi@linux.alibaba.com
Subject: Re: [PATCHv4 3/3] ext4: Move to shared i_rwsem even without dioread_nolock mount opt
Date: Thu, 5 Dec 2019 19:11:36 +0530 [thread overview]
Message-ID: <20191205134137.7C9ED4203F@d06av24.portsmouth.uk.ibm.com> (raw)
In-Reply-To: <20191205120529.GB32639@quack2.suse.cz>
On 12/5/19 5:35 PM, Jan Kara wrote:
> On Thu 05-12-19 12:16:24, Ritesh Harjani wrote:
>> We were using shared locking only in case of dioread_nolock mount option in case
>> of DIO overwrites. This mount condition is not needed anymore with current code,
>> since:-
>>
>> 1. No race between buffered writes & DIO overwrites. Since buffIO writes takes
>> exclusive lock & DIO overwrites will take shared locking. Also DIO path will
>> make sure to flush and wait for any dirty page cache data.
>>
>> 2. No race between buffered reads & DIO overwrites, since there is no block
>> allocation that is possible with DIO overwrites. So no stale data exposure
>> should happen. Same is the case between DIO reads & DIO overwrites.
>>
>> 3. Also other paths like truncate is protected, since we wait there for any DIO
>> in flight to be over.
>>
>> 4. In case of buffIO writes followed by DIO reads:- since here also we take
>> exclusive lock in ext4_write_begin/end(). There is no risk of exposing any
>> stale data in this case. Since after ext4_write_end, iomap_dio_rw() will flush &
>> wait for any dirty page cache data to be written.
>
> The case 4) doesn't seem to be relevant for this patch anymore? Otherwise
> the patch looks good to me. You can add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>
>
Yup, will remove it. Thanks.
> Honza
>
>> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
>> ---
>> fs/ext4/file.c | 9 +++------
>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
>> index cbafaec9e4fc..682ed956eb02 100644
>> --- a/fs/ext4/file.c
>> +++ b/fs/ext4/file.c
>> @@ -392,8 +392,8 @@ static const struct iomap_dio_ops ext4_dio_write_ops = {
>> * - For extending writes case we don't take the shared lock, since it requires
>> * updating inode i_disksize and/or orphan handling with exclusive lock.
>> *
>> - * - shared locking will only be true mostly with overwrites in dioread_nolock
>> - * mode. Otherwise we will switch to exclusive i_rwsem lock.
>> + * - shared locking will only be true mostly with overwrites. Otherwise we will
>> + * switch to exclusive i_rwsem lock.
>> */
>> static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
>> bool *ilock_shared, bool *extend)
>> @@ -415,14 +415,11 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
>> *extend = true;
>> /*
>> * Determine whether the IO operation will overwrite allocated
>> - * and initialized blocks. If so, check to see whether it is
>> - * possible to take the dioread_nolock path.
>> - *
>> + * and initialized blocks.
>> * We need exclusive i_rwsem for changing security info
>> * in file_modified().
>> */
>> if (*ilock_shared && (!IS_NOSEC(inode) || *extend ||
>> - !ext4_should_dioread_nolock(inode) ||
>> !ext4_overwrite_io(inode, offset, count))) {
>> inode_unlock_shared(inode);
>> *ilock_shared = false;
>> --
>> 2.21.0
>>
next prev parent reply other threads:[~2019-12-05 13:41 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-05 6:46 [PATCHv4 0/3] Fix inode_lock sequence to scale performance of DIO mixed R/W workload Ritesh Harjani
2019-12-05 6:46 ` [PATCHv4 1/3] ext4: fix ext4_dax_read/write inode locking sequence for IOCB_NOWAIT Ritesh Harjani
2019-12-05 6:46 ` [PATCHv4 2/3] ext4: Start with shared i_rwsem in case of DIO instead of exclusive Ritesh Harjani
2019-12-05 12:03 ` Jan Kara
2019-12-05 13:40 ` Ritesh Harjani
2019-12-05 6:46 ` [PATCHv4 3/3] ext4: Move to shared i_rwsem even without dioread_nolock mount opt Ritesh Harjani
2019-12-05 12:05 ` Jan Kara
2019-12-05 13:41 ` Ritesh Harjani [this message]
2019-12-05 13:46 ` Ritesh Harjani
2019-12-06 8:46 ` [PATCHv4 0/3] Fix inode_lock sequence to scale performance of DIO mixed R/W workload Joseph Qi
2019-12-06 8:49 ` Ritesh Harjani
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191205134137.7C9ED4203F@d06av24.portsmouth.uk.ibm.com \
--to=riteshh@linux.ibm.com \
--cc=jack@suse.cz \
--cc=joseph.qi@linux.alibaba.com \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=mbobrowski@mbobrowski.org \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).