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@xxxxxxx>
>>>
>>> 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)
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.
Regards,
Ryusuke Konishi
>
> /**
> * 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
