On Mon 10-09-12 11:31:26, Jan Kara wrote:
> On Sun 09-09-12 21:27:10, Dmitry Monakhov wrote:
> > Inode's block defrag and ext4_change_inode_journal_flag() may
> > affect nonlocked DIO reads result, so proper synchronization
> > required.
> >
> > - Add missed inode_dio_wait() calls where appropriate
> > - Check inode state under extra i_dio_count reference.
> >
> > Changes from V1:
> > - add missed memory bariers
> > - move DIOREAD_LOCK state check out from generic should_dioread_nolock
> > otherwise it will affect existing DIO readers.
> Looks good. Just maybe I would rename ext4_inode_dio_wait() to
> ext4_inode_block_unlocked_dio(). Anyway you can add:
> Reviewed-by: Jan Kara <jack@xxxxxxx>
Oh, I found one more thing:
> > +/* Wait for existing dio workers
> > + * Disable DIO read nolock optimization, so new dioreaders will
> > + * be forced to grab i_mutex
> > +*/
> > +void ext4_inode_dio_wait(struct inode *inode, int resume_readers)
> > +{
> > + ext4_set_inode_state(inode, EXT4_STATE_DIOREAD_LOCK);
> > + smp_mb();
> > + inode_dio_wait(inode);
> > + if (resume_readers)
> > + ext4_set_inode_state(inode, EXT4_STATE_DIOREAD_LOCK);
> > +}
> > +
I've just now noticed the resume_readers argument.
a) You probably meant to call ext4_clear_inode_state() when that argument
is set?
b) You need a smp_mb() before clearing DIOREAD_LOCK?
c) Please remove the resume_readers argument and make a separate function
from that. That new function could be called:
ext4_inode_unblock_unlocked_dio().
Honza
> > /*
> > * O_DIRECT for ext3 (or indirect map) based files
> > *
> > @@ -810,10 +823,20 @@ retry:
> > if (unlikely(!list_empty(&ei->i_completed_io_list)))
> > ext4_flush_completed_IO(inode);
> >
> > + /* Nolock dioread optimization may be dynamically disabled.
> > + * Check inode's state while holding extra i_dio_count ref. */
> > + atomic_inc(&inode->i_dio_count);
> > + smp_mb();
> > + if (!unlikely(ext4_test_inode_state(inode,
> > + EXT4_STATE_DIOREAD_LOCK))) {
> > + inode_dio_done(inode);
> > + goto retry;
> > + }
> > ret = __blockdev_direct_IO(rw, iocb, inode,
> > inode->i_sb->s_bdev, iov,
> > offset, nr_segs,
> > ext4_get_block, NULL, NULL, 0);
> > + inode_dio_done(inode);
> > } else {
> > ret = blockdev_direct_IO(rw, iocb, inode, iov,
> > offset, nr_segs, ext4_get_block);
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index 762b955..ffb4a27 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -4720,6 +4720,9 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
> > return err;
> > }
> >
> > + /* Wait for all existing dio workers */
> > + ext4_inode_dio_wait(inode, 0);
> > +
> > jbd2_journal_lock_updates(journal);
> >
> > /*
> > @@ -4739,6 +4742,7 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
> > ext4_set_aops(inode);
> >
> > jbd2_journal_unlock_updates(journal);
> > + ext4_clear_inode_state(inode, EXT4_STATE_DIOREAD_LOCK);
> >
> > /* Finally we can mark the inode as dirty. */
> >
> > diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> > index c5826c6..aea0e7a 100644
> > --- a/fs/ext4/move_extent.c
> > +++ b/fs/ext4/move_extent.c
> > @@ -1214,6 +1214,9 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
> > if (ret1 < 0)
> > return ret1;
> >
> > + /* Wait for all existing dio workers */
> > + ext4_inode_dio_wait(orig_inode, 0);
> > + ext4_inode_dio_wait(donor_inode, 0);
> > /* Protect extent tree against block allocations via delalloc */
> > double_down_write_data_sem(orig_inode, donor_inode);
> > /* Check the filesystem environment whether move_extent can be done */
> > @@ -1412,6 +1415,8 @@ out:
> > kfree(holecheck_path);
> > }
> > double_up_write_data_sem(orig_inode, donor_inode);
> > + ext4_clear_inode_state(orig_inode, EXT4_STATE_DIOREAD_LOCK);
> > + ext4_clear_inode_state(donor_inode, EXT4_STATE_DIOREAD_LOCK);
> > ret2 = mext_inode_double_unlock(orig_inode, donor_inode);
> >
> > if (ret1)
> > --
> > 1.7.7.6
> >
> --
> Jan Kara <jack@xxxxxxx>
> SUSE Labs, CR
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
