| Message ID | 1506543239-31470-3-git-send-email-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27.09.2017 23:13, Jens Axboe wrote: > We currently it it for find_or_create_page(), which means that it nit: Perhaps you wanted to write "We currently use it for find_..." otherwise: Reviewed-by: Nikolay Borisov <nborisov@suse.com> > cannot fail. Ensure we also pass in 'retry == true' to > alloc_page_buffers(), which also ensure that it cannot fail. > > After this, there are no failure cases in grow_dev_page() that > occur because of a failed memory allocation. > > Signed-off-by: Jens Axboe <axboe@kernel.dk> > --- > fs/buffer.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/fs/buffer.c b/fs/buffer.c > index 1234ae343aef..3b60cd8456db 100644 > --- a/fs/buffer.c > +++ b/fs/buffer.c > @@ -988,8 +988,6 @@ grow_dev_page(struct block_device *bdev, sector_t block, > gfp_mask |= __GFP_NOFAIL; > > page = find_or_create_page(inode->i_mapping, index, gfp_mask); > - if (!page) > - return ret; > > BUG_ON(!PageLocked(page)); > > @@ -1008,9 +1006,7 @@ grow_dev_page(struct block_device *bdev, sector_t block, > /* > * Allocate some buffers for this page > */ > - bh = alloc_page_buffers(page, size, false); > - if (!bh) > - goto failed; > + bh = alloc_page_buffers(page, size, true); > > /* > * Link the page to the buffers and initialise them. Take the >
On 09/28/2017 04:11 PM, Nikolay Borisov wrote: > > > On 27.09.2017 23:13, Jens Axboe wrote: >> We currently it it for find_or_create_page(), which means that it > > nit: Perhaps you wanted to write "We currently use it for find_..." > > otherwise: > > Reviewed-by: Nikolay Borisov <nborisov@suse.com> Yeah, fixed up, thanks.
On Wed 27-09-17 14:13:49, Jens Axboe wrote: > We currently it it for find_or_create_page(), which means that it > cannot fail. Ensure we also pass in 'retry == true' to > alloc_page_buffers(), which also ensure that it cannot fail. > > After this, there are no failure cases in grow_dev_page() that > occur because of a failed memory allocation. > > Signed-off-by: Jens Axboe <axboe@kernel.dk> Makes sense. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/buffer.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/fs/buffer.c b/fs/buffer.c > index 1234ae343aef..3b60cd8456db 100644 > --- a/fs/buffer.c > +++ b/fs/buffer.c > @@ -988,8 +988,6 @@ grow_dev_page(struct block_device *bdev, sector_t block, > gfp_mask |= __GFP_NOFAIL; > > page = find_or_create_page(inode->i_mapping, index, gfp_mask); > - if (!page) > - return ret; > > BUG_ON(!PageLocked(page)); > > @@ -1008,9 +1006,7 @@ grow_dev_page(struct block_device *bdev, sector_t block, > /* > * Allocate some buffers for this page > */ > - bh = alloc_page_buffers(page, size, false); > - if (!bh) > - goto failed; > + bh = alloc_page_buffers(page, size, true); > > /* > * Link the page to the buffers and initialise them. Take the > -- > 2.7.4 >
On Tue 03-10-17 14:10:49, Jan Kara wrote: > On Wed 27-09-17 14:13:49, Jens Axboe wrote: > > We currently it it for find_or_create_page(), which means that it > > cannot fail. Ensure we also pass in 'retry == true' to > > alloc_page_buffers(), which also ensure that it cannot fail. > > > > After this, there are no failure cases in grow_dev_page() that > > occur because of a failed memory allocation. > > > > Signed-off-by: Jens Axboe <axboe@kernel.dk> > > Makes sense. You can add: > > Reviewed-by: Jan Kara <jack@suse.cz> I forgot one question though: > > page = find_or_create_page(inode->i_mapping, index, gfp_mask); > > - if (!page) > > - return ret; Are we sure find_or_create_page() cannot fail for other reasons than ENOMEM? Currently it seems to be the case AFAICT but it isn't obvious to me that is guaranteed in future as well... Honza
On 10/03/2017 06:25 AM, Jan Kara wrote: > On Tue 03-10-17 14:10:49, Jan Kara wrote: >> On Wed 27-09-17 14:13:49, Jens Axboe wrote: >>> We currently it it for find_or_create_page(), which means that it >>> cannot fail. Ensure we also pass in 'retry == true' to >>> alloc_page_buffers(), which also ensure that it cannot fail. >>> >>> After this, there are no failure cases in grow_dev_page() that >>> occur because of a failed memory allocation. >>> >>> Signed-off-by: Jens Axboe <axboe@kernel.dk> >> >> Makes sense. You can add: >> >> Reviewed-by: Jan Kara <jack@suse.cz> > > I forgot one question though: > >>> page = find_or_create_page(inode->i_mapping, index, gfp_mask); >>> - if (!page) >>> - return ret; > > Are we sure find_or_create_page() cannot fail for other reasons than > ENOMEM? Currently it seems to be the case AFAICT but it isn't obvious to me > that is guaranteed in future as well... It looks up a page, or allocates and adds one. If we tell the allocator that we cannot tolerate failure, then I don't see what else would be able to make it fail. That function is such an integral part of lookup/create for the page cache.
On Tue 03-10-17 08:36:16, Jens Axboe wrote: > On 10/03/2017 06:25 AM, Jan Kara wrote: > > On Tue 03-10-17 14:10:49, Jan Kara wrote: > >> On Wed 27-09-17 14:13:49, Jens Axboe wrote: > >>> We currently it it for find_or_create_page(), which means that it > >>> cannot fail. Ensure we also pass in 'retry == true' to > >>> alloc_page_buffers(), which also ensure that it cannot fail. > >>> > >>> After this, there are no failure cases in grow_dev_page() that > >>> occur because of a failed memory allocation. > >>> > >>> Signed-off-by: Jens Axboe <axboe@kernel.dk> > >> > >> Makes sense. You can add: > >> > >> Reviewed-by: Jan Kara <jack@suse.cz> > > > > I forgot one question though: > > > >>> page = find_or_create_page(inode->i_mapping, index, gfp_mask); > >>> - if (!page) > >>> - return ret; > > > > Are we sure find_or_create_page() cannot fail for other reasons than > > ENOMEM? Currently it seems to be the case AFAICT but it isn't obvious to me > > that is guaranteed in future as well... > > It looks up a page, or allocates and adds one. If we tell the allocator > that we cannot tolerate failure, then I don't see what else would be > able to make it fail. That function is such an integral part of > lookup/create for the page cache. Well, there memcg stuff in there, radix tree handling etc. So it is not only an allocation that could fail. But all take care of not failing if GFP_NOFAIL is set so I guess there are good chances for this to hold even in the future. Honza
diff --git a/fs/buffer.c b/fs/buffer.c index 1234ae343aef..3b60cd8456db 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -988,8 +988,6 @@ grow_dev_page(struct block_device *bdev, sector_t block, gfp_mask |= __GFP_NOFAIL; page = find_or_create_page(inode->i_mapping, index, gfp_mask); - if (!page) - return ret; BUG_ON(!PageLocked(page)); @@ -1008,9 +1006,7 @@ grow_dev_page(struct block_device *bdev, sector_t block, /* * Allocate some buffers for this page */ - bh = alloc_page_buffers(page, size, false); - if (!bh) - goto failed; + bh = alloc_page_buffers(page, size, true); /* * Link the page to the buffers and initialise them. Take the
We currently it it for find_or_create_page(), which means that it cannot fail. Ensure we also pass in 'retry == true' to alloc_page_buffers(), which also ensure that it cannot fail. After this, there are no failure cases in grow_dev_page() that occur because of a failed memory allocation. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- fs/buffer.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)