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.roh...@gmx.net> >>>> >>>> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html