> As we discussed, we have a challenge where we need DM to avoid issuing
> a barrier before the discard IFF a target doesn't support the discard
> (which the barrier is paired with).
>
> My understanding is that blkdev_issue_discard() only cares if the
> discard was supported. Barrier is used just to decorate the discard
> (for correctness). So by returning -EOPNOTSUPP we're saying the discard
> isn't supported; we're not making any claims about the implict barrier,
> so best to avoid the barrier entirely.
>
> Otherwise we'll be issuing unnecessary barriers (and associated
> performance loss).
>
> So yet another TODO item... Anyway:
>
> Acked-by: Mike Snitzer <[email protected]>
Unnecessary barriers are issued anyway. With each freed extent.
The code must issue a "SYNCHRONIZE CACHE" to flush cache for previous
writes, then "UNMAP" and then another "SYNCHRONIZE CACHE" to commit that
unmap to disk. And this in loop for all extents in
"release_blocks_on_commit".
One idea behind "discard barriers" was to submit a discard request and not
wait for it. Then the request would need a barrier so that it doesn't get
reordered with further writes (that may potentially write to the same area
as the discarded area). But discard isn't used this way anyway,
sb_issue_discard waits for completion, so the barrier isn't needed.
Even if ext4 developers wanted asynchronous discard requests, they should
fire all the discards at once and then submit one zero-sized barrier. Not
barrier with each discard request.
This is up to ext4 developers to optimize and remove the barriers and we
can't do anything with it. Just send "SYNCHRONIZE
CACHE"+"UNMAP"+"SYNCHRONIZE CACHE" like the barrier specification wants...
Mikulas
On Fri, 2 Jul 2010, Mikulas Patocka wrote:
> > As we discussed, we have a challenge where we need DM to avoid issuing
> > a barrier before the discard IFF a target doesn't support the discard
> > (which the barrier is paired with).
> >
> > My understanding is that blkdev_issue_discard() only cares if the
> > discard was supported. Barrier is used just to decorate the discard
> > (for correctness). So by returning -EOPNOTSUPP we're saying the discard
> > isn't supported; we're not making any claims about the implict barrier,
> > so best to avoid the barrier entirely.
> >
> > Otherwise we'll be issuing unnecessary barriers (and associated
> > performance loss).
> >
> > So yet another TODO item... Anyway:
> >
> > Acked-by: Mike Snitzer <[email protected]>
>
> Unnecessary barriers are issued anyway. With each freed extent.
>
> The code must issue a "SYNCHRONIZE CACHE" to flush cache for previous
> writes, then "UNMAP" and then another "SYNCHRONIZE CACHE" to commit that
> unmap to disk. And this in loop for all extents in
> "release_blocks_on_commit".
>
> One idea behind "discard barriers" was to submit a discard request and not
> wait for it. Then the request would need a barrier so that it doesn't get
> reordered with further writes (that may potentially write to the same area
> as the discarded area). But discard isn't used this way anyway,
> sb_issue_discard waits for completion, so the barrier isn't needed.
>
> Even if ext4 developers wanted asynchronous discard requests, they should
> fire all the discards at once and then submit one zero-sized barrier. Not
> barrier with each discard request.
>
> This is up to ext4 developers to optimize and remove the barriers and we
> can't do anything with it. Just send "SYNCHRONIZE
> CACHE"+"UNMAP"+"SYNCHRONIZE CACHE" like the barrier specification wants...
>
> Mikulas
BTW. I understand that the current dm implementation will send two useless
consecutive "SYNCHRONIZE CACHE" commands discard is directed to the part
of the device that doesn't support it.
But the problem is that when you use discard on a part of the device that
supports discard, it also sends two useless "SYNCHRONIZE CACHE" commands
--- they are useless for functionality, but mandated by the barrier
specification.
The fix is supposedly this:
---
include/linux/blkdev.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6.35-rc3-fast/include/linux/blkdev.h
===================================================================
--- linux-2.6.35-rc3-fast.orig/include/linux/blkdev.h 2010-07-02 21:59:21.000000000 +0200
+++ linux-2.6.35-rc3-fast/include/linux/blkdev.h 2010-07-02 21:59:37.000000000 +0200
@@ -1021,7 +1021,7 @@ static inline int sb_issue_discard(struc
block <<= (sb->s_blocksize_bits - 9);
nr_blocks <<= (sb->s_blocksize_bits - 9);
return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_KERNEL,
- BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER);
+ BLKDEV_IFL_WAIT);
}
extern int blk_verify_command(unsigned char *cmd, fmode_t has_write_perm);
> ---
> include/linux/blkdev.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: linux-2.6.35-rc3-fast/include/linux/blkdev.h
> ===================================================================
> --- linux-2.6.35-rc3-fast.orig/include/linux/blkdev.h 2010-07-02 21:59:21.000000000 +0200
> +++ linux-2.6.35-rc3-fast/include/linux/blkdev.h 2010-07-02 21:59:37.000000000 +0200
> @@ -1021,7 +1021,7 @@ static inline int sb_issue_discard(struc
> block <<= (sb->s_blocksize_bits - 9);
> nr_blocks <<= (sb->s_blocksize_bits - 9);
> return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_KERNEL,
> BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER);
> }
>
> extern int blk_verify_command(unsigned char *cmd, fmode_t has_write_perm);
>
A note for ext4 developers: is that GFP_KERNEL safe? Can't it recurse back
to ext4 and attempt to flush more data?
I'm not familiar enough with ext4 to declare that it is/isn't a bug, but
it looks suspicious.
Mikulas
On Fri, Jul 02 2010 at 3:49pm -0400,
Mikulas Patocka <[email protected]> wrote:
> > As we discussed, we have a challenge where we need DM to avoid issuing
> > a barrier before the discard IFF a target doesn't support the discard
> > (which the barrier is paired with).
> >
> > My understanding is that blkdev_issue_discard() only cares if the
> > discard was supported. Barrier is used just to decorate the discard
> > (for correctness). So by returning -EOPNOTSUPP we're saying the discard
> > isn't supported; we're not making any claims about the implict barrier,
> > so best to avoid the barrier entirely.
> >
> > Otherwise we'll be issuing unnecessary barriers (and associated
> > performance loss).
> >
> > So yet another TODO item... Anyway:
> >
> > Acked-by: Mike Snitzer <[email protected]>
>
> Unnecessary barriers are issued anyway. With each freed extent.
>
> The code must issue a "SYNCHRONIZE CACHE" to flush cache for previous
> writes, then "UNMAP" and then another "SYNCHRONIZE CACHE" to commit that
> unmap to disk. And this in loop for all extents in
> "release_blocks_on_commit".
You're delving into the mechanics of the discard when it is supported;
which is fine but tangential to my point above. My point was DM
shouldn't issue any barrier(s) at all if it the discard will not be sent
(because a device doesn't support discards).
> One idea behind "discard barriers" was to submit a discard request and not
> wait for it. Then the request would need a barrier so that it doesn't get
> reordered with further writes (that may potentially write to the same area
> as the discarded area). But discard isn't used this way anyway,
> sb_issue_discard waits for completion, so the barrier isn't needed.
>
> Even if ext4 developers wanted asynchronous discard requests, they should
> fire all the discards at once and then submit one zero-sized barrier. Not
> barrier with each discard request.
sb_issue_discard() is the block layer api that ext4 uses for discards.
Ext4, or any other filesystem that uses sb_issue_discard(), has no
control over the barriers that are issued.
> This is up to ext4 developers to optimize and remove the barriers and we
> can't do anything with it. Just send "SYNCHRONIZE
> CACHE"+"UNMAP"+"SYNCHRONIZE CACHE" like the barrier specification wants...
In practice that is what I see when I remove a file in ext4:
kdmflush-2537 [000] 911436.484481: scsi_dispatch_cmd_start: host_no=5 channel=0 id=0 lun=0 data_sgl=0 prot_sgl=0 cmnd=(SYNCHRONIZE_CACHE - raw=35 00 00 00 00 00 00 00 00 00)
kdmflush-2537 [000] 911436.484482: scsi_dispatch_cmd_done: host_no=5 channel=0 id=0 lun=0 data_sgl=0 prot_sgl=0 cmnd=(SYNCHRONIZE_CACHE - raw=35 00 00 00 00 00 00 00 00 00) result=(driver=DRIVER_OK host=DID_OK message=COMMAND_COMPLETE status=SAM_STAT_GOOD)
kdmflush-2537 [000] 911436.484500: scsi_dispatch_cmd_start: host_no=5 channel=0 id=0 lun=0 data_sgl=1 prot_sgl=0 cmnd=(UNMAP regions=1 raw=42 00 00 00 00 00 00 00 18 00)
<idle>-0 [000] 911436.485238: scsi_dispatch_cmd_done: host_no=5 channel=0 id=0 lun=0 data_sgl=1 prot_sgl=0 cmnd=(UNMAP regions=1 raw=42 00 00 00 00 00 00 00 18 00) result=(driver=DRIVER_OK host=DID_OK message=COMMAND_COMPLETE status=SAM_STAT_GOOD)
kdmflush-2537 [000] 911436.485283: scsi_dispatch_cmd_start: host_no=5 channel=0 id=0 lun=0 data_sgl=0 prot_sgl=0 cmnd=(SYNCHRONIZE_CACHE - raw=35 00 00 00 00 00 00 00 00 00)
kdmflush-2537 [000] 911436.485284: scsi_dispatch_cmd_done: host_no=5 channel=0 id=0 lun=0 data_sgl=0 prot_sgl=0 cmnd=(SYNCHRONIZE_CACHE - raw=35 00 00 00 00 00 00 00 00 00) result=(driver=DRIVER_OK host=DID_OK message=COMMAND_COMPLETE status=SAM_STAT_GOOD)
Mike
On Fri, Jul 02 2010 at 4:00pm -0400,
Mikulas Patocka <[email protected]> wrote:
>
>
> On Fri, 2 Jul 2010, Mikulas Patocka wrote:
>
> > > As we discussed, we have a challenge where we need DM to avoid issuing
> > > a barrier before the discard IFF a target doesn't support the discard
> > > (which the barrier is paired with).
> > >
> > > My understanding is that blkdev_issue_discard() only cares if the
> > > discard was supported. Barrier is used just to decorate the discard
> > > (for correctness). So by returning -EOPNOTSUPP we're saying the discard
> > > isn't supported; we're not making any claims about the implict barrier,
> > > so best to avoid the barrier entirely.
> > >
> > > Otherwise we'll be issuing unnecessary barriers (and associated
> > > performance loss).
> > >
> > > So yet another TODO item... Anyway:
> > >
> > > Acked-by: Mike Snitzer <[email protected]>
> >
> > Unnecessary barriers are issued anyway. With each freed extent.
> >
> > The code must issue a "SYNCHRONIZE CACHE" to flush cache for previous
> > writes, then "UNMAP" and then another "SYNCHRONIZE CACHE" to commit that
> > unmap to disk. And this in loop for all extents in
> > "release_blocks_on_commit".
> >
> > One idea behind "discard barriers" was to submit a discard request and not
> > wait for it. Then the request would need a barrier so that it doesn't get
> > reordered with further writes (that may potentially write to the same area
> > as the discarded area). But discard isn't used this way anyway,
> > sb_issue_discard waits for completion, so the barrier isn't needed.
> >
> > Even if ext4 developers wanted asynchronous discard requests, they should
> > fire all the discards at once and then submit one zero-sized barrier. Not
> > barrier with each discard request.
> >
> > This is up to ext4 developers to optimize and remove the barriers and we
> > can't do anything with it. Just send "SYNCHRONIZE
> > CACHE"+"UNMAP"+"SYNCHRONIZE CACHE" like the barrier specification wants...
> >
> > Mikulas
>
> BTW. I understand that the current dm implementation will send two useless
> consecutive "SYNCHRONIZE CACHE" commands discard is directed to the part
> of the device that doesn't support it.
Issue 1 ^^^
> But the problem is that when you use discard on a part of the device that
> supports discard, it also sends two useless "SYNCHRONIZE CACHE" commands
> --- they are useless for functionality, but mandated by the barrier
> specification.
Issue 2 ^^^
Those are 2 different issues. Please don't join them as if they are one
in the same. DM should treat a discard as a first class request (which
may or may not have a barrier). If a region doesn't support the discard
DM has no business processing anything related to the discard (barriers
included). It is as simple as that.
> The fix is supposedly this:
>
> ---
> include/linux/blkdev.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: linux-2.6.35-rc3-fast/include/linux/blkdev.h
> ===================================================================
> --- linux-2.6.35-rc3-fast.orig/include/linux/blkdev.h 2010-07-02 21:59:21.000000000 +0200
> +++ linux-2.6.35-rc3-fast/include/linux/blkdev.h 2010-07-02 21:59:37.000000000 +0200
> @@ -1021,7 +1021,7 @@ static inline int sb_issue_discard(struc
> block <<= (sb->s_blocksize_bits - 9);
> nr_blocks <<= (sb->s_blocksize_bits - 9);
> return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_KERNEL,
> - BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER);
> + BLKDEV_IFL_WAIT);
> }
>
> extern int blk_verify_command(unsigned char *cmd, fmode_t has_write_perm);
Hmm, older kernels use DISCARD_FL_BARRIER which merely mapped to
BLKDEV_IFL_BARRIER.
Seems you've stumbled onto a bug in the conversion that commit
"blkdev: generalize flags for blkdev_issue_fn functions"
(fbd9b09a177a481eda) performed?
That commit seems to have incorrectly replaced DISCARD_FL_BARRIER with
both: BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER
Dmitry and/or Jens was this intended?
Mike
On Fri, Jul 02, 2010 at 04:47:09PM -0400, Mike Snitzer wrote:
> If a region doesn't support the discard
> DM has no business processing anything related to the discard (barriers
> included). It is as simple as that.
Indeed - if an I/O is going to fail, discover that as early as we can, trying
to avoid the relatively-expensive barrier process whenever we reasonably can.
Alasdair
Mike Snitzer <[email protected]> writes:
> On Fri, Jul 02 2010 at 4:00pm -0400,
> Mikulas Patocka <[email protected]> wrote:
>
>>
>>
>> On Fri, 2 Jul 2010, Mikulas Patocka wrote:
>>
>> > > As we discussed, we have a challenge where we need DM to avoid issuing
>> > > a barrier before the discard IFF a target doesn't support the discard
>> > > (which the barrier is paired with).
>> > >
>> > > My understanding is that blkdev_issue_discard() only cares if the
>> > > discard was supported. Barrier is used just to decorate the discard
>> > > (for correctness). So by returning -EOPNOTSUPP we're saying the discard
>> > > isn't supported; we're not making any claims about the implict barrier,
>> > > so best to avoid the barrier entirely.
>> > >
>> > > Otherwise we'll be issuing unnecessary barriers (and associated
>> > > performance loss).
>> > >
>> > > So yet another TODO item... Anyway:
>> > >
>> > > Acked-by: Mike Snitzer <[email protected]>
>> >
>> > Unnecessary barriers are issued anyway. With each freed extent.
>> >
>> > The code must issue a "SYNCHRONIZE CACHE" to flush cache for previous
>> > writes, then "UNMAP" and then another "SYNCHRONIZE CACHE" to commit that
>> > unmap to disk. And this in loop for all extents in
>> > "release_blocks_on_commit".
>> >
>> > One idea behind "discard barriers" was to submit a discard request and not
>> > wait for it. Then the request would need a barrier so that it doesn't get
>> > reordered with further writes (that may potentially write to the same area
>> > as the discarded area). But discard isn't used this way anyway,
>> > sb_issue_discard waits for completion, so the barrier isn't needed.
>> >
>> > Even if ext4 developers wanted asynchronous discard requests, they should
>> > fire all the discards at once and then submit one zero-sized barrier. Not
>> > barrier with each discard request.
>> >
>> > This is up to ext4 developers to optimize and remove the barriers and we
>> > can't do anything with it. Just send "SYNCHRONIZE
>> > CACHE"+"UNMAP"+"SYNCHRONIZE CACHE" like the barrier specification wants...
>> >
>> > Mikulas
>>
>> BTW. I understand that the current dm implementation will send two useless
>> consecutive "SYNCHRONIZE CACHE" commands discard is directed to the part
>> of the device that doesn't support it.
>
> Issue 1 ^^^
>
>> But the problem is that when you use discard on a part of the device that
>> supports discard, it also sends two useless "SYNCHRONIZE CACHE" commands
>> --- they are useless for functionality, but mandated by the barrier
>> specification.
>
> Issue 2 ^^^
>
> Those are 2 different issues. Please don't join them as if they are one
> in the same. DM should treat a discard as a first class request (which
> may or may not have a barrier). If a region doesn't support the discard
> DM has no business processing anything related to the discard (barriers
> included). It is as simple as that.
>
>> The fix is supposedly this:
>>
>> ---
>> include/linux/blkdev.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Index: linux-2.6.35-rc3-fast/include/linux/blkdev.h
>> ===================================================================
>> --- linux-2.6.35-rc3-fast.orig/include/linux/blkdev.h 2010-07-02 21:59:21.000000000 +0200
>> +++ linux-2.6.35-rc3-fast/include/linux/blkdev.h 2010-07-02 21:59:37.000000000 +0200
>> @@ -1021,7 +1021,7 @@ static inline int sb_issue_discard(struc
>> block <<= (sb->s_blocksize_bits - 9);
>> nr_blocks <<= (sb->s_blocksize_bits - 9);
>> return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_KERNEL,
>> - BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER);
>> + BLKDEV_IFL_WAIT);
>> }
>>
>> extern int blk_verify_command(unsigned char *cmd, fmode_t has_write_perm);
>
> Hmm, older kernels use DISCARD_FL_BARRIER which merely mapped to
> BLKDEV_IFL_BARRIER.
>
> Seems you've stumbled onto a bug in the conversion that commit
> "blkdev: generalize flags for blkdev_issue_fn functions"
> (fbd9b09a177a481eda) performed?
>
> That commit seems to have incorrectly replaced DISCARD_FL_BARRIER with
> both: BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER
>
> Dmitry and/or Jens was this intended?
Yes, before the path WAIT behavior was implicit, now caller is
responsible for exact behavior.
So, as it was mentioned earlier, it is reasonable to send
discard request only with BLKDEV_IFL_BARRIER flag from some places in
ext4. I have optimization patches for that in my queue, i hope they
will be ready soon.
>
> Mike
On Fri, 2 Jul 2010, Mike Snitzer wrote:
> On Fri, Jul 02 2010 at 4:00pm -0400,
> Mikulas Patocka <[email protected]> wrote:
>
> >
> >
> > On Fri, 2 Jul 2010, Mikulas Patocka wrote:
> >
> > > > As we discussed, we have a challenge where we need DM to avoid issuing
> > > > a barrier before the discard IFF a target doesn't support the discard
> > > > (which the barrier is paired with).
> > > >
> > > > My understanding is that blkdev_issue_discard() only cares if the
> > > > discard was supported. Barrier is used just to decorate the discard
> > > > (for correctness). So by returning -EOPNOTSUPP we're saying the discard
> > > > isn't supported; we're not making any claims about the implict barrier,
> > > > so best to avoid the barrier entirely.
> > > >
> > > > Otherwise we'll be issuing unnecessary barriers (and associated
> > > > performance loss).
> > > >
> > > > So yet another TODO item... Anyway:
> > > >
> > > > Acked-by: Mike Snitzer <[email protected]>
> > >
> > > Unnecessary barriers are issued anyway. With each freed extent.
> > >
> > > The code must issue a "SYNCHRONIZE CACHE" to flush cache for previous
> > > writes, then "UNMAP" and then another "SYNCHRONIZE CACHE" to commit that
> > > unmap to disk. And this in loop for all extents in
> > > "release_blocks_on_commit".
> > >
> > > One idea behind "discard barriers" was to submit a discard request and not
> > > wait for it. Then the request would need a barrier so that it doesn't get
> > > reordered with further writes (that may potentially write to the same area
> > > as the discarded area). But discard isn't used this way anyway,
> > > sb_issue_discard waits for completion, so the barrier isn't needed.
> > >
> > > Even if ext4 developers wanted asynchronous discard requests, they should
> > > fire all the discards at once and then submit one zero-sized barrier. Not
> > > barrier with each discard request.
> > >
> > > This is up to ext4 developers to optimize and remove the barriers and we
> > > can't do anything with it. Just send "SYNCHRONIZE
> > > CACHE"+"UNMAP"+"SYNCHRONIZE CACHE" like the barrier specification wants...
> > >
> > > Mikulas
> >
> > BTW. I understand that the current dm implementation will send two useless
> > consecutive "SYNCHRONIZE CACHE" commands discard is directed to the part
> > of the device that doesn't support it.
>
> Issue 1 ^^^
>
> > But the problem is that when you use discard on a part of the device that
> > supports discard, it also sends two useless "SYNCHRONIZE CACHE" commands
> > --- they are useless for functionality, but mandated by the barrier
> > specification.
>
> Issue 2 ^^^
>
> Those are 2 different issues. Please don't join them as if they are one
> in the same. DM should treat a discard as a first class request (which
> may or may not have a barrier).
What I mean --- if you fix Issue 2, Issue 1 is no longer a problem.
> If a region doesn't support the discard
> DM has no business processing anything related to the discard (barriers
> included). It is as simple as that.
You can optimize out the second SYNCHRONIZE CACHE, but not the first one
(because when it is sent, we don't know if the discard will succeed or
not).
Basically, the fix is to prefix the second dm_flush in process_barrier
with if (md->barrier_error != -EOPNOTSUPP).
The "barrier+discard" concept is problematic anyway. If we specify that
"barrier+discard" request doesn't have to do the barrier if discard fails
(as it is currently), then the request is useless to maintain disk
integrity because the discard may fail anytime (and so the barrier).
I think they will eventually remove "barrier+discard" from the filesystems
at all.
Mikulas
Filesystems can call sb_issue_discard on a memory reclaim path
(e.g. ext4 calls sb_issue_discard during journal commit).
Use GFP_NOFS in sb_issue_discard to avoid recursing back into the FS.
Reported-by: Mikulas Patocka <[email protected]>
Signed-off-by: Mike Snitzer <[email protected]>
---
include/linux/blkdev.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index baf5258..dbb510c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -933,7 +933,7 @@ static inline int sb_issue_discard(struct super_block *sb,
{
block <<= (sb->s_blocksize_bits - 9);
nr_blocks <<= (sb->s_blocksize_bits - 9);
- return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_KERNEL,
+ return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_NOFS,
BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER);
}
On Tue, Jul 06, 2010 at 12:11:56PM -0400, Mike Snitzer wrote:
> Filesystems can call sb_issue_discard on a memory reclaim path
> (e.g. ext4 calls sb_issue_discard during journal commit).
>
> Use GFP_NOFS in sb_issue_discard to avoid recursing back into the FS.
>
> Reported-by: Mikulas Patocka <[email protected]>
> Signed-off-by: Mike Snitzer <[email protected]>
Hi Jens,
I never saw an ack from you on this patch. Are you ok with it, and
have you grabbed it for your tree? Do you want me to include this in
the ext4 tree, even though it's a patch to include/linux/blkdev.h?
Thanks,
- Ted
> ---
> include/linux/blkdev.h | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index baf5258..dbb510c 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -933,7 +933,7 @@ static inline int sb_issue_discard(struct super_block *sb,
> {
> block <<= (sb->s_blocksize_bits - 9);
> nr_blocks <<= (sb->s_blocksize_bits - 9);
> - return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_KERNEL,
> + return blkdev_issue_discard(sb->s_bdev, block, nr_blocks, GFP_NOFS,
> BLKDEV_IFL_WAIT | BLKDEV_IFL_BARRIER);
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 27 2010 at 9:44am -0400,
Ted Ts'o <[email protected]> wrote:
> On Tue, Jul 06, 2010 at 12:11:56PM -0400, Mike Snitzer wrote:
> > Filesystems can call sb_issue_discard on a memory reclaim path
> > (e.g. ext4 calls sb_issue_discard during journal commit).
> >
> > Use GFP_NOFS in sb_issue_discard to avoid recursing back into the FS.
> >
> > Reported-by: Mikulas Patocka <[email protected]>
> > Signed-off-by: Mike Snitzer <[email protected]>
>
> Hi Jens,
>
> I never saw an ack from you on this patch. Are you ok with it, and
> have you grabbed it for your tree? Do you want me to include this in
> the ext4 tree, even though it's a patch to include/linux/blkdev.h?
Hi Ted,
Thanks for following up on this. In my experience, Jens is more apt to
pick up a patch if it gets explicitly 'Acked-by' other stake-holders
(especially when a patch is motivated by another subsystem, in this case
the proposed block change addresses a problem unique to fs/ext4).
Mike
On 07/27/2010 05:33 PM, Mike Snitzer wrote:
> On Tue, Jul 27 2010 at 9:44am -0400,
> Ted Ts'o <[email protected]> wrote:
>
>> On Tue, Jul 06, 2010 at 12:11:56PM -0400, Mike Snitzer wrote:
>>> Filesystems can call sb_issue_discard on a memory reclaim path
>>> (e.g. ext4 calls sb_issue_discard during journal commit).
>>>
>>> Use GFP_NOFS in sb_issue_discard to avoid recursing back into the FS.
>>>
>>> Reported-by: Mikulas Patocka <[email protected]>
>>> Signed-off-by: Mike Snitzer <[email protected]>
>>
>> Hi Jens,
>>
>> I never saw an ack from you on this patch. Are you ok with it, and
>> have you grabbed it for your tree? Do you want me to include this in
>> the ext4 tree, even though it's a patch to include/linux/blkdev.h?
>
> Hi Ted,
>
> Thanks for following up on this. In my experience, Jens is more apt to
> pick up a patch if it gets explicitly 'Acked-by' other stake-holders
> (especially when a patch is motivated by another subsystem, in this case
> the proposed block change addresses a problem unique to fs/ext4).
I'll pick this up. I've been away for a few weeks and I'm currently
on vacation, but I'll push this with a few other pending patches
for .35 on monday.
--
Jens Axboe
Confidentiality Notice: This e-mail message, its contents and any attachments to it are confidential to the intended recipient, and may contain information that is privileged and/or exempt from disclosure under applicable law. If you are not the intended recipient, please immediately notify the sender and destroy the original e-mail message and any attachments (and any copies that may have been made) from your system or otherwise. Any unauthorized use, copying, disclosure or distribution of this information is strictly prohibited.
On Wed, Jul 28, 2010 at 08:12:15PM +0200, Jens Axboe wrote:
>
> I'll pick this up. I've been away for a few weeks and I'm currently
> on vacation, but I'll push this with a few other pending patches
> for .35 on monday.
Great, thanks!!
- Ted