Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1286361 > unrolled thread

[RFC PATCH 0/3] debugfs: implement 'debugfs_create_dir_with_tmpfiles()'

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/3] debugfs: implement 'debugfs_create_dir_with_tmpfiles()' Roman Pen <r.peniaev@gmail.com> - 2015-12-08 11:00 +0100
    Re: [RFC PATCH 0/3] debugfs: implement  'debugfs_create_dir_with_tmpfiles()' Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2015-12-09 01:50 +0100
      Re: [RFC PATCH 0/3] debugfs: implement 'debugfs_create_dir_with_tmpfiles()' Roman Peniaev <r.peniaev@gmail.com> - 2015-12-09 21:10 +0100

#1286361 — [RFC PATCH 0/3] debugfs: implement 'debugfs_create_dir_with_tmpfiles()'

Hello.

Here is an attempt to solve annoying race, which exists between two operations
on debugfs entries: write (setting a request) and read (reading a response).

E.g. let's assume that we have some storage device, which can have thousands
of snapshots (yeah, plenty of them, thus it is ridicoulous to create a debugfs
entry for each), and each snapshot is controlled by the handle, which is a UUID
or any non-numeric character sequence (for numeric sequence this problem can be
solved by 'seek' operation).  This device provides a debugfs entry 'snap_status',
which can be opened for reading and writing, where write - is an operation for
specifiying a request, and read - is an operation for getting a response back.

I.e. it is obvious, that to request a status of a snapshot you have to write a
UUID first of a snapshot and then read back a status response back, so the
sequence can be the following:

  # echo $UUID > /sys/kernel/debug/storage/snap_status
  # cat /sys/kernel/debug/storage/snap_status

Between those two operations a race exists, and if someone else comes and
requests status for another snapshot, the first requester will get incorrect
data.

An atomic request-set and response-read solution can be the following:

  # cat /sys/kernel/debug/storage/snap_status/$UUID

Here debugfs creates non-existent temporary entry on demand with the $UUID
name and eventually calls file operations, which were passed to the
'debugfs_create_dir_with_tmpfiles()' function.  Caller of that function can
control the correctness of the file name in 'i_fop->open' callback and can
return an error if temporary file name does not match some format.

Temporary file, which is created, will not appear in any lookups, further
linking is forbidden, corresponding dentry and inode will be freed when last
file descriptor is closed (see O_TMPFILE, with the only difference is that
debugfs temporary dentry has a name).

Of course this file creation on demand can be applied to many other cases,
where it is impossible to create as many debugfs entries as objects exist,
but atomicity of read-write can be required.

This atomicity can be achieved also by locking from userspace, but that approach
increases complexity and makes it hardly possible to invoke only few commands
from command line, like 'echo' or 'cat'.

So basically creating a temporary file on demand with a specified name is a
way to provide one additional parameter for an 'read' operation.

Probably, there is more elegant solution for that write-read race problem,
but I've not found any.

PS. I did not want to use configfs, because I have nothing to configure (what
    I have described is not a configuration issue), and I do not like to keep
	dentries in a system if userspace forgets to remove them.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org

Roman Pen (3):
  debugfs: make create directory logic more generic
  debugfs: implement 'debugfs_create_dir_with_tmpfiles()'
  debugfs: update some bits of documentation

 Documentation/filesystems/debugfs.txt |  25 ++++++
 fs/debugfs/inode.c                    | 157 ++++++++++++++++++++++++++++++----
 include/linux/debugfs.h               |  12 +++
 3 files changed, 179 insertions(+), 15 deletions(-)

-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1286945 — Re: [RFC PATCH 0/3] debugfs: implement 'debugfs_create_dir_with_tmpfiles()'

On Tue, Dec 08, 2015 at 10:51:03AM +0100, Roman Pen wrote:
> Hello.
> 
> Here is an attempt to solve annoying race, which exists between two operations
> on debugfs entries: write (setting a request) and read (reading a response).
> 
> E.g. let's assume that we have some storage device, which can have thousands
> of snapshots (yeah, plenty of them, thus it is ridicoulous to create a debugfs
> entry for each), and each snapshot is controlled by the handle, which is a UUID
> or any non-numeric character sequence (for numeric sequence this problem can be
> solved by 'seek' operation).  This device provides a debugfs entry 'snap_status',
> which can be opened for reading and writing, where write - is an operation for
> specifiying a request, and read - is an operation for getting a response back.
> 
> I.e. it is obvious, that to request a status of a snapshot you have to write a
> UUID first of a snapshot and then read back a status response back, so the
> sequence can be the following:
> 
>   # echo $UUID > /sys/kernel/debug/storage/snap_status
>   # cat /sys/kernel/debug/storage/snap_status
> 
> Between those two operations a race exists, and if someone else comes and
> requests status for another snapshot, the first requester will get incorrect
> data.
> 
> An atomic request-set and response-read solution can be the following:
> 
>   # cat /sys/kernel/debug/storage/snap_status/$UUID
> 
> Here debugfs creates non-existent temporary entry on demand with the $UUID
> name and eventually calls file operations, which were passed to the
> 'debugfs_create_dir_with_tmpfiles()' function.  Caller of that function can
> control the correctness of the file name in 'i_fop->open' callback and can
> return an error if temporary file name does not match some format.
> 
> Temporary file, which is created, will not appear in any lookups, further
> linking is forbidden, corresponding dentry and inode will be freed when last
> file descriptor is closed (see O_TMPFILE, with the only difference is that
> debugfs temporary dentry has a name).
> 
> Of course this file creation on demand can be applied to many other cases,
> where it is impossible to create as many debugfs entries as objects exist,
> but atomicity of read-write can be required.
> 
> This atomicity can be achieved also by locking from userspace, but that approach
> increases complexity and makes it hardly possible to invoke only few commands
> from command line, like 'echo' or 'cat'.
> 
> So basically creating a temporary file on demand with a specified name is a
> way to provide one additional parameter for an 'read' operation.
> 
> Probably, there is more elegant solution for that write-read race problem,
> but I've not found any.
> 
> PS. I did not want to use configfs, because I have nothing to configure (what
>     I have described is not a configuration issue), and I do not like to keep
> 	dentries in a system if userspace forgets to remove them.

Do you have a patch series that depends on these new apis?  I don't want
to add things to debugfs without any in-tree users if at all possible.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1287802

On Tue, Dec 8, 2015 at 12:49 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Dec 08, 2015 at 10:51:03AM +0100, Roman Pen wrote:
>> Hello.
>>
>> Here is an attempt to solve annoying race, which exists between two operations
>> on debugfs entries: write (setting a request) and read (reading a response).
>>
>> E.g. let's assume that we have some storage device, which can have thousands
>> of snapshots (yeah, plenty of them, thus it is ridicoulous to create a debugfs
>> entry for each), and each snapshot is controlled by the handle, which is a UUID
>> or any non-numeric character sequence (for numeric sequence this problem can be
>> solved by 'seek' operation).  This device provides a debugfs entry 'snap_status',
>> which can be opened for reading and writing, where write - is an operation for
>> specifiying a request, and read - is an operation for getting a response back.
>>
[skipped]
>>
>> So basically creating a temporary file on demand with a specified name is a
>> way to provide one additional parameter for an 'read' operation.
>>
>> Probably, there is more elegant solution for that write-read race problem,
>> but I've not found any.
>>
>> PS. I did not want to use configfs, because I have nothing to configure (what
>>     I have described is not a configuration issue), and I do not like to keep
>>       dentries in a system if userspace forgets to remove them.
>
> Do you have a patch series that depends on these new apis?  I don't want
> to add things to debugfs without any in-tree users if at all possible.

I can show only similar write/read usage, which I tried to avoid.  I
did the grep and found
the following files which do exactly what I've described here:

     linux/drivers/bluetooth/btmrvl_debufgfs.c
        .read   = btmrvl_hscfgcmd_read,
        .write  = btmrvl_hscfgcmd_write,

        .read = btmrvl_pscmd_read,
        .write = btmrvl_pscmd_write,

        .read   = btmrvl_hscmd_read,
        .write  = btmrvl_hscmd_write,

     linux/drivers/mfd/ab8500-debugfs.c
        .open = ab8500_bank_open,
        .write = ab8500_bank_write,

        .open = ab8500_address_open,
        .write = ab8500_address_write,

        .open = ab8500_val_open,
        .write = ab8500_val_write,


     linux/drivers/mmc/card/mmc_test.c
        .open       = mtf_test_open,
        .read       = seq_read,
        .write      = mtf_test_write,

     linux/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c
        .read =  ixgbe_dbg_reg_ops_read,
        .write = ixgbe_dbg_reg_ops_write,

        .read = ixgbe_dbg_netdev_ops_read,
        .write = ixgbe_dbg_netdev_ops_write,

     linux/drivers/platform/olpc/olpc-ec.c
        .write = ec_dbgfs_cmd_write,
        .read = ec_dbgfs_cmd_read,

     linux/kernel/time/test_udelay.c
        .open = udelay_test_open,
        .read = seq_read,
        .write = udelay_test_write,


Of course, I could miss something, because plenty of callers with
similar meaning,
but in the list above everything boils down to setting request on
write() and getting
response back on read().

For example this simplest and representative test_udelay.c:

     echo "100 10" >   debugfs/udelay_test
     cat  debugfs/udelay_test

can be replaced with atomic sequence:
    cat  debugfs/udelay_test/"100 10"

And frankly I do not know does it make sense to switch these functions
to new API
and to break userspace expectations, but for sure they are the
candidates to behave
atomically.

--
Roman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web