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-03 02:35, Ryusuke Konishi wrote:
> On Mon, 01 Sep 2014 21:18:30 +0200, Andreas Rohner wrote:
>> 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?
> 
> I believe compiler doesn't reorder set_bit() operation after an
> external function call unless it knows the content of the function and
> the function can be optimized.  But, yes, set_bit() doesn't imply
> memory barrier unlike test_and_set_bit().  As for
> blkdev_issue_flush(), it would imply memory barrier by some lock
> functions or other primitive used inside it.  (I haven't actually
> confirmed that the premise is true)

Yes blkdev_issue_flush() probably implies a memory barrier.

> On the other hand, we need explicit barrier operation like
> smp_mb__after_atomic() if a certain operation is performed after
> set_bit() and the changed bit should be visible to other processors
> before the operation.

Great suggestion. I didn't know about those functions. Do we also need a
call to smp_mb__before_atomic() before clear_nilfs_flushed(nilfs) in
segment.c?

I would be happy to provide another version of the patch with
set_nilfs_flushed(nilfs) and smp_mb__after_atomic() if you prefer that
version over the test_and_set_bit approach...

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