Christoph Hellwig <hch@xxxxxxxxxxxxx> writes:
>> + trace_iomap_dio_rw_begin(iocb, iter, dio_flags, done_before, ret);
>> dio = __iomap_dio_rw(iocb, iter, ops, dops, dio_flags, private,
>> done_before);
>> if (IS_ERR_OR_NULL(dio)) {
>> @@ -689,6 +691,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>> }
>> ret = iomap_dio_complete(dio);
>> out:
>> + trace_iomap_dio_rw_end(iocb, iter, dio_flags, done_before, ret);
>
> The trace_iomap_dio_rw_end tracepoint heere seems a bit weird,
> and we'll miss it for file systems using __iomap_dio_rw directly.
Sorry, yes you are right.
>
> I'd instead add a trace_iomap_dio_rw_queued for the case where
> __iomap_dio_rw returns ERR_PTR(-EIOCBQUEUED), as otherwise we're
> nicely covered by the complete trace points.
>
How about this below change? Does this look good to you?
It should cover all error types and both entry and exit.
And should I fold this entire change in 1 patch or should I split the
refactoring of common out routine into a seperate one?
diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 5871956ee880..e412fdc4ee86 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -130,6 +130,7 @@ ssize_t iomap_dio_complete(struct iomap_dio *dio)
if (ret > 0)
ret += dio->done_before;
+ trace_iomap_dio_complete(iocb, dio->error, ret);
kfree(dio);
return ret;
@@ -493,12 +494,15 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
struct blk_plug plug;
struct iomap_dio *dio;
+ trace_iomap_dio_rw_begin(iocb, iter, dio_flags, done_before, ret);
if (!iomi.len)
- return NULL;
+ goto out;
dio = kmalloc(sizeof(*dio), GFP_KERNEL);
- if (!dio)
- return ERR_PTR(-ENOMEM);
+ if (!dio) {
+ ret = -ENOMEM;
+ goto out;
+ }
dio->iocb = iocb;
atomic_set(&dio->ref, 1);
@@ -650,8 +654,11 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
*/
dio->wait_for_completion = wait_for_completion;
if (!atomic_dec_and_test(&dio->ref)) {
- if (!wait_for_completion)
- return ERR_PTR(-EIOCBQUEUED);
+ if (!wait_for_completion) {
+ ret = -EIOCBQUEUED;
+ goto out;
+ }
+
for (;;) {
set_current_state(TASK_UNINTERRUPTIBLE);
@@ -663,10 +670,13 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
__set_current_state(TASK_RUNNING);
}
+ trace_iomap_dio_rw_end(iocb, iter, dio_flags, done_before, ret);
return dio;
out_free_dio:
kfree(dio);
+out:
+ trace_iomap_dio_rw_end(iocb, iter, dio_flags, done_before, ret);
if (ret)
return ERR_PTR(ret);
return NULL;
>> + __print_flags(__entry->dio_flags, "|", TRACE_IOMAP_DIO_STRINGS),
>
> Please avoid the overly lone line here.
Somehow my checkpatch never gave a warning about it.
I will check why was that. But yes, I have anyways made the name to
IOMAP_DIO_STRINGS similar to other namings used in fs/iomap/trace.h
-ritesh
