* [PATCH] fs: Move check for mappings without pages from iterate_bdevs()
@ 2012-09-25 9:22 Jan Kara
2012-09-27 5:28 ` Al Viro
0 siblings, 1 reply; 3+ messages in thread
From: Jan Kara @ 2012-09-25 9:22 UTC (permalink / raw)
To: Al Viro; +Cc: linux-fsdevel, Jan Kara
Currently, iterate_bdevs() skips block devices without any pages in page
cache. That is fine for current use by sync(2) but may be rather surprising
for possible future users. So move the checks from iterate_bdevs() to
callback functions used by sync(2).
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/block_dev.c | 5 +----
fs/sync.c | 12 ++++++++++--
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 38e721b..1631cbd 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1720,11 +1720,8 @@ void iterate_bdevs(void (*func)(struct block_device *, void *), void *arg)
spin_lock(&inode_sb_list_lock);
list_for_each_entry(inode, &blockdev_superblock->s_inodes, i_sb_list) {
- struct address_space *mapping = inode->i_mapping;
-
spin_lock(&inode->i_lock);
- if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW) ||
- mapping->nrpages == 0) {
+ if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) {
spin_unlock(&inode->i_lock);
continue;
}
diff --git a/fs/sync.c b/fs/sync.c
index eb8722d..536a272 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -81,12 +81,20 @@ static void sync_fs_one_sb(struct super_block *sb, void *arg)
static void fdatawrite_one_bdev(struct block_device *bdev, void *arg)
{
- filemap_fdatawrite(bdev->bd_inode->i_mapping);
+ struct address_space *mapping = bdev->bd_inode->i_mapping;
+
+ if (mapping->nrpages == 0)
+ return;
+ filemap_fdatawrite(mapping);
}
static void fdatawait_one_bdev(struct block_device *bdev, void *arg)
{
- filemap_fdatawait(bdev->bd_inode->i_mapping);
+ struct address_space *mapping = bdev->bd_inode->i_mapping;
+
+ if (mapping->nrpages == 0)
+ return;
+ filemap_fdatawait(mapping);
}
/*
--
1.7.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] fs: Move check for mappings without pages from iterate_bdevs()
2012-09-25 9:22 [PATCH] fs: Move check for mappings without pages from iterate_bdevs() Jan Kara
@ 2012-09-27 5:28 ` Al Viro
2012-09-27 9:31 ` Jan Kara
0 siblings, 1 reply; 3+ messages in thread
From: Al Viro @ 2012-09-27 5:28 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-fsdevel
On Tue, Sep 25, 2012 at 11:22:07AM +0200, Jan Kara wrote:
> Currently, iterate_bdevs() skips block devices without any pages in page
> cache. That is fine for current use by sync(2) but may be rather surprising
> for possible future users. So move the checks from iterate_bdevs() to
> callback functions used by sync(2).
*snort*
You know, testing is occasionally useful. Sure, it's boring, but once in
a while one gets amusing results. The thing is, the only reason why the
sucker hadn't oopsed *without* that patch was that the only non-bdev on that
inode list happened to have zero in ->mapping->nr_pages. Reliably.
What we'd accidentally avoided (until that patch) was stepping on the
root directory of bdev filesystem. I_BDEV() on it is fine - it's allocated
in a regular way, so we are not doing anything bad with container_of() here.
However, it never went through bdget(), obviously - just new_inode(). And
it has I_BDEV(inode)->bd_inode == NULL. The rest is obious...
More interesting question is whether inode list is the right approach. After
all, there's a list with suggestive name - all_bdevs...
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] fs: Move check for mappings without pages from iterate_bdevs()
2012-09-27 5:28 ` Al Viro
@ 2012-09-27 9:31 ` Jan Kara
0 siblings, 0 replies; 3+ messages in thread
From: Jan Kara @ 2012-09-27 9:31 UTC (permalink / raw)
To: Al Viro; +Cc: Jan Kara, linux-fsdevel
On Thu 27-09-12 06:28:37, Al Viro wrote:
> On Tue, Sep 25, 2012 at 11:22:07AM +0200, Jan Kara wrote:
> > Currently, iterate_bdevs() skips block devices without any pages in page
> > cache. That is fine for current use by sync(2) but may be rather surprising
> > for possible future users. So move the checks from iterate_bdevs() to
> > callback functions used by sync(2).
>
> *snort*
>
> You know, testing is occasionally useful. Sure, it's boring, but once in
> a while one gets amusing results.
Heh, stupid me. Sorry for that. The patch looked *so* obviously correct
;)...
> The thing is, the only reason why the
> sucker hadn't oopsed *without* that patch was that the only non-bdev on that
> inode list happened to have zero in ->mapping->nr_pages. Reliably.
> What we'd accidentally avoided (until that patch) was stepping on the
> root directory of bdev filesystem. I_BDEV() on it is fine - it's allocated
> in a regular way, so we are not doing anything bad with container_of() here.
> However, it never went through bdget(), obviously - just new_inode(). And
> it has I_BDEV(inode)->bd_inode == NULL. The rest is obious...
>
> More interesting question is whether inode list is the right approach. After
> all, there's a list with suggestive name - all_bdevs...
And I completely missed that list. It looks like a better match for my
needs. I'll change the function to iterate over that list. Thanks for the
pointer.
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-09-27 9:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-25 9:22 [PATCH] fs: Move check for mappings without pages from iterate_bdevs() Jan Kara
2012-09-27 5:28 ` Al Viro
2012-09-27 9:31 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).