Re: [PATCH v2 1/1] nilfs2: add missing blkdev_issue_flush() to nilfs_sync_fs()

[Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>]

On 2014-09-01 20:43, Andreas Rohner wrote:
> Hi Ryusuke,
> On 2014-09-01 19:59, Ryusuke Konishi wrote:
>> On Sun, 31 Aug 2014 17:47:13 +0200, Andreas Rohner wrote:
>>> Under normal circumstances nilfs_sync_fs() writes out the super block,
>>> which causes a flush of the underlying block device. But this depends on
>>> the THE_NILFS_SB_DIRTY flag, which is only set if the pointer to the
>>> last segment crosses a segment boundary. So if only a small amount of
>>> data is written before the call to nilfs_sync_fs(), no flush of the
>>> block device occurs.
>>>
>>> In the above case an additional call to blkdev_issue_flush() is needed.
>>> To prevent unnecessary overhead, the new flag THE_NILFS_FLUSHED is
>>> introduced, which is cleared whenever new logs are written and set
>>> whenever the block device is flushed.
>>>
>>> Signed-off-by: Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
>>
>> The patch looks good to me except that I feel the use of atomic
>> test-and-set bitwise operations something unfavorable (though it's
>> logically correct).  I will try to send this to upstream as is unless
>> a comment comes to mind.
> 
> I originally thought, that it is necessary to do it atomically to avoid
> a race condition, but I am not so sure about that any more. I think the
> only case we have to avoid is, to call set_nilfs_flushed() after
> blkdev_issue_flush(), because this could race with the
> clear_nilfs_flushed() from the segment construction. So this should also
> work:
> 
>  +	if (wait && !err && nilfs_test_opt(nilfs, BARRIER) &&
>  +	    !nilfs_flushed(nilfs)) {
>  +              set_nilfs_flushed(nilfs);
>  +		err = blkdev_issue_flush(sb->s_bdev, GFP_KERNEL, NULL);
>  +		if (err != -EIO)
>  +			err = 0;
>  +	}
>  +

On the other hand, it says in the comments to set_bit(), that it can be
reordered on architectures other than x86. test_and_set_bit() implies a
memory barrier on all architectures. But I don't think the processor
would reorder set_nilfs_flushed() after the external function call to
blkdev_issue_flush(), would it?

/**
 * set_bit - Atomically set a bit in memory
 * @nr: the bit to set
 * @addr: the address to start counting from
 *
 * This function is atomic and may not be reordered.  See __set_bit()
 * if you do not require the atomic guarantees.
 *
 * Note: there are no guarantees that this function will not be reordered
 * on non x86 architectures, so if you are writing portable code,
 * make sure not to rely on its reordering guarantees.
 */

br,
Andreas Rohner
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html